[LowerTypeTests] Skip declarations when determining Thumb support

[arichardson]

When looping over all functions in a module to determine whether any of
them is built with support for B.W, we can skip declarations since those
do not have an associated target-feature attribute.
This was found by the assertion from #129600

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Alexander Richardson (arichardson)

Changes

When looping over all functions in a module to determine whether any of
them is built with support for B.W, we can skip intrinsics since those
do not have an associated target-feature attribute.

---

Full diff: https://github.com/llvm/llvm-project/pull/129599.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/IPO/LowerTypeTests.cpp (+4)

diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index e3caefe70311b..85cfa42fdc94f 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -1847,6 +1847,10 @@ LowerTypeTestsModule::LowerTypeTestsModule(
     auto &FAM =
         AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
     for (Function &F : M) {
+      // Skip intrinsics since we should not query the TTI for them.
+      // TODO: Can we skip all declarations and only look at definitions?
+      if (F.isIntrinsic())
+        continue;
       auto &TTI = FAM.getResult<TargetIRAnalysis>(F);
       if (TTI.hasArmWideBranch(false))
         CanUseArmJumpTable = true;

[efriedma-quic]

This code was last revisited in a8cd35c, I think.

We should only need to visit functions that have definitions; the idea here is to figure out the supported subtargets based on code we're actually generating. The whole logic here is a bit dubious though, given that runtime feature detection exists on A-class targets. Probably more of this should be driven by the target triple.

[arichardson]

This code was last revisited in a8cd35c, I think.

We should only need to visit functions that have definitions; the idea here is to figure out the supported subtargets based on code we're actually generating. The whole logic here is a bit dubious though, given that runtime feature detection exists on A-class targets. Probably more of this should be driven by the target triple.

That's what I wasn't sure about - thanks. I'll update this to just use definitions.

[statham-arm]

The whole logic here is a bit dubious though, given that runtime feature detection exists on A-class targets. Probably more of this should be driven by the target triple.

If the target triple reliably contained enough information to distinguish Armv6-M from later versions, I agree! I only did this horrible thing at all because I couldn't see any other way to find out that specific detail. I agree that a module including functions with elevated target attributes, intending not to call them unless runtime feature detection permits, would confuse this logic, and I'd love it if there were a way to find out the same fact that's both simpler and more reliable.

[statham-arm]

@arichardson, the patch LGTM, but I presume you'll edit the title and description to say "declarations" rather than "intrinsics" after the patch itself was modified?

[efriedma-quic]

If the target triple reliably contained enough information to distinguish Armv6-M from later versions, I agree!

clang generally rewrites the triple in a way that makes querying the subarch from the triple reliable. Not sure if we expect non-clang frontends to do the same thing.

[statham-arm]

clang generally rewrites the triple in a way that makes querying the subarch from the triple reliable. Not sure if we expect non-clang frontends to do the same thing.

I don't know either 🙂

I can imagine in some cases even that might not be enough. For this particular purpose the subarch is all that's needed, but if we needed to query a particular SubtargetFeature to decide whether a certain instruction was safe to put in jump tables, surely the target triple wouldn't include that much detail.

I don't think that's come up yet. In this same module there is the question of whether to put the Arm bti instruction in jump tables, but those are safe, because they turn into NOPs on architectures that don't support them. But I wouldn't rule out something like that in future.

I must say that if I were considering cleaning this code up, I'd start by trying to move the generation of detailed machine code jump tables into the backend, where it surely belongs, and where all the necessary information is already conveniently available! Having to do it by emitting asm into the IR at a much earlier stage in the pipeline is surely not ideal, and if I remember rightly, the only reason it has to be done this way is because the offsets between jump table entries have to end up as constants in the IR that refers to the jump table. My instinct would be to look for a solution to that, before trying to find ways to make the bodgy asm generation itself more reliable.

But someone else's closed PR is also not the best place to discuss it 🙂