[ARM] Inline callee if its target-features are a subset of the caller

[fhahn]

Summary

Similar to X86, it should be safe to inline callees if their
target-features are a subset of the caller. As some subtarget features
provide different instructions depending on whether they are set or
unset (e.g. ThumbMode and ModeSoftFloat), we use a whitelist of
target-features describing hardware capabilities only.

[fhahn]

Ping, any thoughts on this? I committed a similar patch for AArch64 recently (D34698).

[SjoerdMeijer]

This looks like a straight forward and sensible implementation of a target hook to me. But before committing, perhaps someone with more inlining experience than me can have a quick look as well.

[fhahn]

Thanks Sjoerd! I'll hold of committing this until tomorrow so people in other timezones can voice concerns.

[efriedma]

lib/Target/ARM/ARMTargetTransformInfo.cpp

31

Are you sure ModeThumb is the only feature we need to check for? ModeSoftFloat in particular seems suspicious: VFP intrinsics in the callee could fail to compile. (I'd be much happier if we were whitelisting features; I'm afraid of missing something.)

[fhahn]

Use whitelist, excludes ModeThumb, ModeSoftFloat and FeatureNoARM.

[fhahn]

lib/Target/ARM/ARMTargetTransformInfo.cpp

31

Thanks for pointing out ModeSoftFloat, which behaves similar to ModeThumb. They both force different modes, with different instructions available, depending whether they are set or unset.

I think ideally they shouldn't be subtarget features, as my understanding was that subtarget features should only provide additional capabilities/instructions when they are enabled. But I guess that would a far bigger change and a whitelist should be fine for now.

[efriedma]

lib/Target/ARM/ARMTargetTransformInfo.cpp

34

ARM::FeatureD16 and FeatureVFPOnlySP are also suspicious... not because they're different modes, but because having the feature enabled does the opposite of what you would expect.

[fhahn]

Removed FeatureD16 and FeatureVFPOnlySP from the whitelist, thanks again @efriedma . I've also moved the whitelist to ARMTargetTransformInfo::InlineFeatureWhitelist so it does only get instantiated once.

I'll hold off committing this until tomorrow, to give people a chance to raise concerns with moving the whitelist to a member variable in ARMTargetTransformInfo.

lib/Target/ARM/ARMTargetTransformInfo.h

 class ARMTTIImpl : public BasicTTIImplBase<ARMTTIImpl> {
   typedef BasicTTIImplBase<ARMTTIImpl> BaseT;
   typedef TargetTransformInfo TTI;
   friend BaseT;
 
   const ARMSubtarget *ST;
   const ARMTargetLowering *TLI;
 
+  // Currently the following features are excluded from InlineFeatureWhitelist.
+  // ModeThumb, FeatureNoARM, ModeSoftFloat, FeatureVFPOnlySP, FeatureD16
+  // Depending on whether they are set or unset, different
+  // instructions/registers are available. For example, inlining a callee with
+  // -thumb-mode in a caller with +thumb-mode, may cause the assembler to
+  // fail if the callee uses ARM only instructions, e.g. in inline asm.
+  const FeatureBitset InlineFeatureWhitelist = {
+      ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureNEON, ARM::FeatureThumb2,
+      ARM::FeatureFP16, ARM::FeatureVFP4, ARM::FeatureFPARMv8,
+      ARM::FeatureFullFP16, ARM::FeatureHWDivThumb,
+      ARM::FeatureHWDivARM, ARM::FeatureDB, ARM::FeatureV7Clrex,
+      ARM::FeatureAcquireRelease, ARM::FeatureSlowFPBrcc,
+      ARM::FeaturePerfMon, ARM::FeatureTrustZone, ARM::Feature8MSecExt,
+      ARM::FeatureCrypto, ARM::FeatureCRC, ARM::FeatureRAS,
+      ARM::FeatureFPAO, ARM::FeatureFuseAES, ARM::FeatureZCZeroing,
+      ARM::FeatureProfUnpredicate, ARM::FeatureSlowVGETLNi32,
+      ARM::FeatureSlowVDUP32, ARM::FeaturePreferVMOVSR,
+      ARM::FeaturePrefISHSTBarrier, ARM::FeatureMuxedUnits,
+      ARM::FeatureSlowOddRegister, ARM::FeatureSlowLoadDSubreg,
+      ARM::FeatureDontWidenVMOVS, ARM::FeatureExpandMLx,
+      ARM::FeatureHasVMLxHazards, ARM::FeatureNEONForFPMovs,
+      ARM::FeatureNEONForFP, ARM::FeatureCheckVLDnAlign,
+      ARM::FeatureHasSlowFPVMLx, ARM::FeatureVMLxForwarding,
+      ARM::FeaturePref32BitThumb, ARM::FeatureAvoidPartialCPSR,
+      ARM::FeatureCheapPredicableCPSR, ARM::FeatureAvoidMOVsShOp,
+      ARM::FeatureHasRetAddrStack, ARM::FeatureHasNoBranchPredictor,
+      ARM::FeatureDSP, ARM::FeatureMP, ARM::FeatureVirtualization,
+      ARM::FeatureMClass, ARM::FeatureRClass, ARM::FeatureAClass,
+      ARM::FeatureNaClTrap, ARM::FeatureStrictAlign, ARM::FeatureLongCalls,
+      ARM::FeatureExecuteOnly, ARM::FeatureReserveR9, ARM::FeatureNoMovt,
+      ARM::FeatureNoNegativeImmediates
+  };
+
   const ARMSubtarget *getST() const { return ST; }
   const ARMTargetLowering *getTLI() const { return TLI; }
 
 public:
   explicit ARMTTIImpl(const ARMBaseTargetMachine *TM, const Function &F)
       : BaseT(TM, F.getParent()->getDataLayout()), ST(TM->getSubtargetImpl(F)),
         TLI(ST->getTargetLowering()) {}
 
+  bool areInlineCompatible(const Function *Caller,
+                           const Function *Callee) const;
+
   bool enableInterleavedAccessVectorization() { return true; }
 
   /// Floating-point computation using ARMv8 AArch32 Advanced
   /// SIMD instructions remains unchanged from ARMv7. Only AArch64 SIMD
   /// is IEEE-754 compliant, but it's not covered in this target.
   bool isFPVectorizationPotentiallyUnsafe() {
     return !ST->isTargetDarwin();
   }

lib/Target/ARM/ARMTargetTransformInfo.cpp

 #include "ARMTargetTransformInfo.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Target/CostTable.h"
 #include "llvm/Target/TargetLowering.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "armtti"
 
+bool ARMTTIImpl::areInlineCompatible(const Function *Caller,
+                                     const Function *Callee) const {
+  const TargetMachine &TM = getTLI()->getTargetMachine();
+  const FeatureBitset &CallerBits =
+      TM.getSubtargetImpl(*Caller)->getFeatureBits();
+  const FeatureBitset &CalleeBits =
+      TM.getSubtargetImpl(*Callee)->getFeatureBits();
+
+  // To inline a callee, all features not in the whitelist must match exactly.
+  bool MatchExact = (CallerBits & ~InlineFeatureWhitelist) ==
+                    (CalleeBits & ~InlineFeatureWhitelist);
+  // For features in the whitelist, the callee's features must be a subset of
+  // the callers'.
+  bool MatchSubset = ((CallerBits & CalleeBits) & InlineFeatureWhitelist) ==
+                     (CalleeBits & InlineFeatureWhitelist);
+  return MatchExact && MatchSubset;
+}
+
 int ARMTTIImpl::getIntImmCost(const APInt &Imm, Type *Ty) {
   assert(Ty->isIntegerTy());
 
  unsigned Bits = Ty->getPrimitiveSizeInBits();
  if (Bits == 0 || Imm.getActiveBits() >= 64)
    return 4;
 
   int64_t SImmVal = Imm.getSExtValue();

test/Transforms/Inline/ARM/inline-target-attr.ll

+; RUN: opt < %s -mtriple=arm-unknown-linux-gnu -S -inline | FileCheck %s
+; RUN: opt < %s -mtriple=arm-unknown-linux-gnu -S -passes='cgscc(inline)' | FileCheck %s
+; Check that we only inline when we have compatible target attributes.
+; ARM has implemented a target attribute that will verify that the attribute
+; sets are compatible.
+
+define i32 @foo() #0 {
+entry:
+  %call = call i32 (...) @baz()
+  ret i32 %call
+; CHECK-LABEL: foo
+; CHECK: call i32 (...) @baz()
+}
+declare i32 @baz(...) #0
+
+define i32 @bar() #1 {
+entry:
+  %call = call i32 @foo()
+  ret i32 %call
+; CHECK-LABEL: bar
+; CHECK: call i32 (...) @baz()
+}
+
+define i32 @qux() #0 {
+entry:
+  %call = call i32 @bar()
+  ret i32 %call
+; CHECK-LABEL: qux
+; CHECK: call i32 @bar()
+}
+
+define i32 @thumb_fn() #2 {
+entry:
+  %call = call i32 @foo()
+  ret i32 %call
+; CHECK-LABEL: thumb_fn
+; CHECK: call i32 @foo
+}
+
+define i32 @strict_align() #3 {
+entry:
+  %call = call i32 @foo()
+  ret i32 %call
+; CHECK-LABEL: strict_align
+; CHECK: call i32 (...) @baz()
+}
+
+define i32 @soft_float_fn() #4 {
+entry:
+  %call = call i32 @foo()
+  ret i32 %call
+; CHECK-LABEL: thumb_fn
+; CHECK: call i32 @foo
+}
+
+attributes #0 = { "target-cpu"="generic" "target-features"="+dsp,+neon" }
+attributes #1 = { "target-cpu"="generic" "target-features"="+dsp,+neon,+fp16" }
+attributes #2 = { "target-cpu"="generic" "target-features"="+dsp,+neon,+fp16,+thumb-mode" }
+attributes #3 = { "target-cpu"="generic" "target-features"="+dsp,+neon,+strict-align" }
+attributes #4 = { "target-cpu"="generic" "target-features"="+dsp,+neon,+fp16,+soft-float" }

test/Transforms/Inline/ARM/lit.local.cfg

+if not 'ARM' in config.root.targets:
+    config.unsupported = True