We are in the middle of upgrading our local LLVM and some upstream changes have exposed odd/inconsistent behavior in opt and llc.
This all got exposed by commit
9c54ee437898314f956cfc048d9038ca775ea692 which added setIntSize()/getIntSize() to TargetLibraryInfo. setIntSize ends up being called with a value depending on the Triple, using the Triple’s pointer size to infer sizeof(int). Note that I do not consider 9c54ee437898314f956cfc048d9038ca775ea692 to be a faulty change, though it is perhaps dangerous to assume sizeof(int) is the same as the pointer size.
We got some test failures due to the fact that sizeof(int) was inferred with the wrong value. I traced the cause back to being an empty Triple when TargetLibraryInfo was initialized. This surprised me, as both -march and -mcpu were explicitly given on the test command line. The IR file does not have a triple in it because we use the same IR to test multiple targets.
opt apparently doesn’t try to guess the target/Triple at all from -march/-mcpu. It happily runs without a TargetMachine, but it does initialize TargetTransformInfo with an empty Triple, which I think could lead to dangerous code optimization.
llc is in some sense worse. It initializes a TargetMachine from -march/-mcpu but leaves the Triple undefined. So code gets generated for the specified target, but with potentially completely wrong information in TargetLibraryInfo. The two things are inconsistent.
Is this the expected behavior of these tools? Should the Triple be inferred from other options if possible? If not, is the user expected to pass a triple option explicitly? I am trying to determine if I should add a triple option to all of our tests or if I should implement inferring the triple from -march/-mcpu before passing it to TargetLibraryInfo. The latter seems safer to me but I am cognizant that sometimes inferring things can get rather complex and brittle.
In ancient history, LLVM supported running opt on “generic” IR, without any triple or datalayout. We gave up on IR without a datalayout ages ago. I guess IR without a triple is the remaining legacy of that…
The thing I’d be most concerned about, if we’re forcing a default, is existing IR tests using “opt”; we have a lot of “generic” tests that don’t specify a datalayout or triple. For such tests, we use a “default” datalayout which vaguely resembles common 64-bit targets. For the triple, there isn’t any obvious default to use. Probably the vast majority of tests would work fine with any triple, but implicitly using the host triple seems likely to lead to regression test failures for non-x86 targets.
For TargetLibraryInfo specifically, it probably makes sense to add a fallback: if there’s no target triple, force freestanding mode, where we don’t recognize any functions. Then the size of int shouldn’t matter.
I’m surprised llc isn’t setting the target triple; I think we already force the datalayout, and compute a target triple. So it should be very easy to just set the triple we’ve already computed.
Indeed, the llc behavior was most surprising to me and is what triggered our test failures. I just looked at opt out of an abundance of caution. It makes sense to me to fallback as you indicated if opt doesn’t have a triple. There’s already a disableAllFunctions method on TargetLibraryInfo and it seems easy enough to invoke that in the constructor if the Triple is empty. Would that be an acceptable approach? I can submit a patch if so. If there’s a better way to do it I can figure that out/get some community guidance.
Setting the target triple in llc turned out to be almost trivial, at least to solve our issue but I’m not 100% certain it is general enough. I can submit the patch if there is interest.
I missed a subtlety in your response that I want to address:
As I wrote earlier, an opt fallback for TargetLibraryInfo makes sense to me if there truly is no triple. The remaining question I have is what should happen if there is no triple but -march/-mcpu are specified on the command line? Should we infer a triple from that? As a user I would expect that if I gave -march/-mcpu opt would apply machine-dependent transformations as appropriate.
If we want to infer the triple, I’m not entirely sure how to do it in a robust way. In our copy I simply get the default triple (which we configured via CMake to be our target) and then opt’s calling of TargetRegistry::lookupTarget naturally adjusts it to fit whatever -march/-mcpu tells it. I don’t know how well that works if LLVM_DEFAULT_TARGET_TRIPLE is something wildly different from what is implied by -march/-mcpu (e.g. LLVM_DEFAULT_TARGET_TRIPLE is the host machine but -march/-mcpu is some cross-compilation). From the code in lookupTarget it seems like the architecture component is updated but not the vendor or OS (since -march/-mcpu doesn’t imply anything about those). That could be…less than ideal.
I’d appreciate your thoughts on this point. Thanks!
Currently, as far as I can tell, if you don’t specify a triple, opt ignores -march/-mcpu ? The backend flags are only used to configure the TargetMachine, and we skip creating a TargetMachine if the triple doesn’t specify a known architecture. I don’t see any reason we need to change that.
That’s right. I’m wondering if that is the correct behavior. It sounds like there are some complications with changing that and this is probably best left as later work.