Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace lli with %lli in RUN lines #38872

Open
llvmbot opened this issue Nov 1, 2018 · 4 comments
Open

Replace lli with %lli in RUN lines #38872

llvmbot opened this issue Nov 1, 2018 · 4 comments
Labels
bugzilla Issues migrated from bugzilla test-suite

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 1, 2018

Bugzilla Link 39524
Version trunk
OS Linux
Attachments WIP patch for converting all tests to %lli
Reporter LLVM Bugzilla Contributor
CC @dwblaikie,@MatzeB,@rnk

Extended Description

lit.cfg.py defines substitutions for both lli and %lli, and this is
confusing, especially since there is no actual difference between these two
except that on Windows %lli includes an `-mtriple=' flag force-setting ELF
object format.

There is even a FIXME in `test/lit.cfg.py' about this very thing:

FIXME: Why do we have both lli and %lli that do slightly different

things?

It seems like the majority of tests could be converted to use one or the other.
If all tests can be converted to one substitution, the other one should be removed.
If there is a minority of tests that depend on the other substitution,
that substitution should be renamed to something special a la %llc_dwarf to
clearly indicate that it is a special case and not the default.

I included a patch that converts everything to %lli, but I don't know if it
really works because I don't have a Windows machine to test it on.
@MatzeB
Copy link
Contributor

MatzeB commented Nov 1, 2018

FWIW: It seems that for most tools shipping with llvm (llc, opt, etc.) we just use the version with a '%' prefix. I think those are listed in the tools.extend list.

I recommend putting the patch up on phabricator and subscribing some people that are active on windows to the review.

@MatzeB
Copy link
Contributor

MatzeB commented Nov 1, 2018

we just use the version with a '%' prefix.

That should have been: ... the version without a '%' prefix

@rnk
Copy link
Collaborator

rnk commented Nov 2, 2018

we just use the version with a '%' prefix.

That should have been: ... the version without a '%' prefix

+1

@llvmbot
Copy link
Collaborator Author

llvmbot commented Nov 3, 2018

we just use the version with a '%' prefix.

That should have been: ... the version without a '%' prefix

That's because there is no '%'-prefixed version of llc, opt, etc.

The majority of tests that run lli use %lli (~170 vs ~18), and I think it
should stay that way if an -mtriple flag is required because otherwise
(if the flag is attached to lli in lit.cfg.py) there will be no simple way
of setting a different triple for a test run since -mtriple can occur at most
once, and it may also seem just more confusing to people.

I recommend putting the patch up on phabricator and subscribing some people
that are active on windows to the review.

Thank you for advice, but I'm not sure. The WIP patch was just a simple
search-and-replace job because on my machine %lli and lli are the same.
Most of work is still to be done, and I can't contribute any of it. If the
thinking is that this little issue won't get enough attention from the right
people here, would you think posting on llvm-dev will help? If you still think
posting this patch on Phabricator is more appropriate I will do it.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla test-suite
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rnk @MatzeB @llvmbot