[Inliner] Don't inline functions with incompatible vscale_range attributes

[c-rhodes]

Summary

Inlining a function with a vscale_range attribute that is more
restrictive than the caller's vscale_range could lead to incorrect
results, prevent it in the inliner.

Inlining a function with a less restrictive vscale_range into a function
with a more restrictive vscale_range is ok, and potentially profitable
since the compiler has more opportunity to optimise.

[sdesmalen]

llvm/lib/Transforms/Utils/InlineFunction.cpp

1747–1761

nit: the code below says as much, so the comment is a bit redundant?

1764

nit: you can query the attribute even if it is not set, since it returns a invalid/empty Attribute. This way you can write:

Attribute CallerRange = Caller.getFnAttribute(Attribute::VScaleRange);
Attribute CalleeRange = Callee.getFnAttribute(Attribute::VScaleRange);

unsigned CallerMinVScale, CallerMaxVScale;
std::tie(CallerMinVScale, CallerMaxVScale) = CallerRange.getValueAsBool()
        ? CallerRange.getVScaleRangeArgs()
        : {0, 0};
unsigned CalleeMinVScale, CalleeMaxVScale;
std::tie(CalleeMinVScale, CalleeMaxVScale) = CalleeRange.getValueAsBool()
        ? CalleeRange.getVScaleRangeArgs()
        : {0, 0};

And then do all the tests on the individual min and max ranges, rather than having to discard the 'has no attribute' cases first.

1773

I'm wondering if this is overly restrictive. If the caller has vscale_range(4, 4) and the callee has no vscale_range (i.e. is unbounded, meaning the code runs on machine with any vscale), then I think it can still be inlined because if it can run on a machine with any vscale, it can also run on a machine with vscale_range(4, 4)

1794

Currently this code would avoid inlining a function with

`vscale_range(0, 16)` into a function with `vscale_range(4, 16)`

which we should probably allow.

For the 'unbounded' minimum I think we can interpret this as '1' (i.e. the minimally required vscale could be anything, it means it (also) has to run on a vscale of 1, so we can assume 1).
For the unbounded maximum we can interpret this as UINT_MAX (i.e. the length of the vector is unknown, so assume worst-case)

And then do the individual comparisons on those newly interpreted values.

[david-arm]

llvm/lib/Transforms/Utils/InlineFunction.cpp

1794

I wonder what the point is of having a 0 for the minimum here then? It feels like we should just never allow a 0 minimum for vscale_range as I don't think the LangRef is particularly clear. The way I read the LangRef it sounds like min could be anything, like 8 or 3. If min could be >4 then it's not safe to inline, right? At the moment it feels like we're tightening the meaning of the vscale_range attribute without changing the documentation, which feels a bit dangerous that's all.

[sdesmalen]

llvm/lib/Transforms/Utils/InlineFunction.cpp

1794

You're right that 'unbounded' is a strange value for the minimum and I recall conversations where we'd like to see this fixed because it doesn't really have any other meaning than '1'.

What it means is that if the minimally required vscale is unbounded, the program with attribute vscale_range(0, xx) has to run on a machine with a value of vscale that's at least 1, or 2, or ... , or K. The lowest requirement for the minimum here is for vscale to be at least '1', so we can assume 1.

[c-rhodes]

Address comments

[c-rhodes]

llvm/lib/Transforms/Utils/InlineFunction.cpp

1747–1761

nit: the code below says as much, so the comment is a bit redundant?

I've removed the comment.

1764

nit: you can query the attribute even if it is not set, since it returns a invalid/empty Attribute. This way you can write:

Attribute CallerRange = Caller.getFnAttribute(Attribute::VScaleRange);
Attribute CalleeRange = Callee.getFnAttribute(Attribute::VScaleRange);

unsigned CallerMinVScale, CallerMaxVScale;
std::tie(CallerMinVScale, CallerMaxVScale) = CallerRange.getValueAsBool()
        ? CallerRange.getVScaleRangeArgs()
        : {0, 0};
unsigned CalleeMinVScale, CalleeMaxVScale;
std::tie(CalleeMinVScale, CalleeMaxVScale) = CalleeRange.getValueAsBool()
        ? CalleeRange.getVScaleRangeArgs()
        : {0, 0};

And then do all the tests on the individual min and max ranges, rather than having to discard the 'has no attribute' cases first.

getValueAsBool expects the attribute value to be a string so I couldn't use that, I've opted to check .isValid() instead.

1794

Currently this code would avoid inlining a function with

`vscale_range(0, 16)` into a function with `vscale_range(4, 16)`

which we should probably allow.

For the 'unbounded' minimum I think we can interpret this as '1' (i.e. the minimally required vscale could be anything, it means it (also) has to run on a vscale of 1, so we can assume 1).

Thanks for pointing this out, should be fixed now. I've added tests for unbounded min.

For the unbounded maximum we can interpret this as UINT_MAX (i.e. the length of the vector is unknown, so assume worst-case)

And then do the individual comparisons on those newly interpreted values.

I've mapped unbounded max to UINT_MAX.

[sdesmalen]

Thanks, that looks a lot simpler now!

llvm/test/Transforms/Inline/inline-vscale-range.ll

5

It may be easier to read the tests if you move the CHECK: remark:'s to the caller definitions.
Can you also enable -pass-remarks=inline and have CHECK lines to check that a function has been inlined (for the positive cases) ?

21

why are these CHECK lines here?

[c-rhodes]

llvm/test/Transforms/Inline/inline-vscale-range.ll

5

It may be easier to read the tests if you move the CHECK: remark:'s to the caller definitions.

All the remarks are emitted at the top, I don't think this will work with the CHECK-LABELs but I'll try.

Can you also enable -pass-remarks=inline and have CHECK lines to check that a function has been inlined (for the positive cases) ?

Sure

21

why are these CHECK lines here?

Because it's not inlined, these are auto-generated

[c-rhodes]

add checks for -pass-remarks=inline

[c-rhodes]

llvm/test/Transforms/Inline/inline-vscale-range.ll

5

I've added positive tests. I agree it would be nice to have the remark CHECKs beside the relevant functions but it doesn't like that. I looked at some loop vectorizer test (scalable-vf-hint.ll) I recalled from a while back and there we have 2 run lines so the remarks are inline, bit I'm not sure if that's worth doing here.

[c-rhodes]

• Drop internal from callee functions, CHECK lines are now generated for all callees.
• Add a separate run line for remarks and move checks beside relevant callers.

[sdesmalen]

LGTM, cheers for the changes @c-rhodes!

[paulwalker-arm]

Hi @c-rhodes , sorry for not getting to this until after you've spent time fixing up review comments but I'm struggling to see what bug this patch is trying to fix and how preventing function inlining fixes it. From what I can see we're basically saying that when function A calls function B and function A's vscale_range is incompatible with function B's then we should not allow B to be inlined into A. But if the two functions have incompatible vscale_range attributes then the actual bug is that A should not be calling B (i.e. it's a bug in user code) and preventing inline will not fix that. What do you think? Have I misunderstood something?

[c-rhodes]

In D111403#3063588, @paulwalker-arm wrote:

Hi @c-rhodes , sorry for not getting to this until after you've spent time fixing up review comments but I'm struggling to see what bug this patch is trying to fix and how preventing function inlining fixes it. From what I can see we're basically saying that when function A calls function B and function A's vscale_range is incompatible with function B's then we should not allow B to be inlined into A. But if the two functions have incompatible vscale_range attributes then the actual bug is that A should not be calling B (i.e. it's a bug in user code) and preventing inline will not fix that. What do you think? Have I misunderstood something?

Thanks for raising this Paul, it seems you're right this isn't actually necessary. It was thought this type of inlining with incompatible vscale_range attributes could occur when using function multi-versioning, but that seems unlikely.

llvm/lib/Transforms/Utils/InlineFunction.cpp

if (InsertRetainCall) {
       Builder.SetInsertPoint(RI);
       Function *IFn = Intrinsic::getDeclaration(Mod, Intrinsic::objc_retain);
       Value *BC = Builder.CreateBitCast(RetOpnd, IFn->getArg(0)->getType());
       Builder.CreateCall(IFn, BC, "");
     }
   }
 }
 
+/// Determine if a caller/callee have incompatible vscale_range attributes to
+/// prevent inlining that could otherwise lead to invalid results.
+static bool haveIncompatibleVScaleRangeAttributes(const Function &Caller,
+                                                  const Function &Callee) {
+  const auto CallerRange = Caller.getFnAttribute(Attribute::VScaleRange);
+  const auto CalleeRange = Callee.getFnAttribute(Attribute::VScaleRange);
+
+  unsigned CallerMinVScale = 0, CallerMaxVScale = 0;
+  if (CallerRange.isValid())
+    std::tie(CallerMinVScale, CallerMaxVScale) =
+        CallerRange.getVScaleRangeArgs();
+  unsigned CalleeMinVScale = 0, CalleeMaxVScale = 0;
+  if (CalleeRange.isValid())
+    std::tie(CalleeMinVScale, CalleeMaxVScale) =
+        CalleeRange.getVScaleRangeArgs();
+
+  if (CallerMaxVScale == 0)
+    CallerMaxVScale = UINT_MAX;
+  if (CalleeMaxVScale == 0)
+    CalleeMaxVScale = UINT_MAX;
+
+  return (CallerMinVScale < CalleeMinVScale) ||
+         (CallerMaxVScale > CalleeMaxVScale);
+}
+
 /// This function inlines the called function into the basic block of the
 /// caller. This returns false if it is not possible to inline this call.
 /// The program is still in a well defined state if this occurs though.
 ///
 /// Note that this only does one level of inlining.  For example, if the
 /// instruction 'call B' is inlined, and 'B' calls 'C', then the call to 'C' now
 /// exists in the instruction stream.  Similarly this will inline a recursive
 /// function by one level.
 llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
                                         AAResults *CalleeAAR,
                                         bool InsertLifetime,
                                         Function *ForwardVarArgsTo) {
   assert(CB.getParent() && CB.getFunction() && "Instruction not in function!");
 
   // FIXME: we don't inline callbr yet.
   if (isa<CallBrInst>(CB))
     return InlineResult::failure("We don't inline callbr yet.");
 
   // If IFI has any state in it, zap it before we fill it in.
   IFI.reset();
 
   Function *CalledFunc = CB.getCalledFunction();
   if (!CalledFunc ||               // Can't inline external function or indirect
       CalledFunc->isDeclaration()) // call!
     return InlineResult::failure("external or indirect");
 
   // The inliner does not know how to inline through calls with operand bundles
   // in general ...
   if (CB.hasOperandBundles()) {
     for (int i = 0, e = CB.getNumOperandBundles(); i != e; ++i) {
       uint32_t Tag = CB.getOperandBundleAt(i).getTagID();
       // ... but it knows how to inline through "deopt" operand bundles ...
llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
   //  2. If the caller has a differing GC, it is invalid to inline.
   if (CalledFunc->hasGC()) {
     if (!Caller->hasGC())
       Caller->setGC(CalledFunc->getGC());
     else if (CalledFunc->getGC() != Caller->getGC())
       return InlineResult::failure("incompatible GC");
   }
 
+  if (haveIncompatibleVScaleRangeAttributes(*Caller, *CalledFunc))
+    return InlineResult::failure("incompatible vscale_range attributes");
+
   // Get the personality function from the callee if it contains a landing pad.
   Constant *CalledPersonality =
       CalledFunc->hasPersonalityFn()
           ? CalledFunc->getPersonalityFn()->stripPointerCasts()
           : nullptr;
 
   // Find the personality function used by the landing pads of the caller. If it
   // exists, then check to see that it matches the personality function used in

llvm/test/Transforms/Inline/inline-vscale-range.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt %s -inline -S | FileCheck %s
+; RUN: opt %s -inline -pass-remarks=inline -pass-remarks-missed=inline -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-REMARK
+
+; Callees
+
+define void @callee_unbounded() vscale_range(0, 0) {
+; CHECK-LABEL: @callee_unbounded(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @callee_unbounded_min() vscale_range(0, 4) {
+; CHECK-LABEL: @callee_unbounded_min(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @callee_restricted_min() vscale_range(2, 4) {
+; CHECK-LABEL: @callee_restricted_min(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @callee_unrestricted_min() vscale_range(1, 4) {
+; CHECK-LABEL: @callee_unrestricted_min(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @callee_unbounded_max() vscale_range(1, 0) {
+; CHECK-LABEL: @callee_unbounded_max(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @callee_restricted_max() vscale_range(1, 8) {
+; CHECK-LABEL: @callee_restricted_max(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @callee_unrestricted_max() vscale_range(1, 16) {
+; CHECK-LABEL: @callee_unrestricted_max(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+; Callers
+
+; CHECK-REMARK: remark: {{.*}} 'callee_unbounded' inlined into 'caller_unbounded__callee_unbounded'
+define void @caller_unbounded__callee_unbounded() vscale_range(0, 0) {
+; CHECK-LABEL: @caller_unbounded__callee_unbounded(
+; CHECK-NEXT:    ret void
+;
+  call void @callee_unbounded()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_unbounded_min' inlined into 'caller_restricted_min__callee_unbounded_min'
+define void @caller_restricted_min__callee_unbounded_min() vscale_range(2, 4) {
+; CHECK-LABEL: @caller_restricted_min__callee_unbounded_min(
+; CHECK-NEXT:    ret void
+;
+  call void @callee_unbounded_min()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_restricted_min' is not inlined into 'caller_unbounded_min__callee_restricted_min': incompatible vscale_range attributes
+define void @caller_unbounded_min__callee_restricted_min() vscale_range(0, 4) {
+; CHECK-LABEL: @caller_unbounded_min__callee_restricted_min(
+; CHECK-NEXT:    call void @callee_restricted_min()
+; CHECK-NEXT:    ret void
+;
+  call void @callee_restricted_min()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_restricted_min' inlined into 'caller_restricted_min__callee_restricted_min'
+define void @caller_restricted_min__callee_restricted_min() vscale_range(2, 4) {
+; CHECK-LABEL: @caller_restricted_min__callee_restricted_min(
+; CHECK-NEXT:    ret void
+;
+  call void @callee_restricted_min()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_restricted_min' is not inlined into 'caller_unrestricted_min__callee_restricted_min': incompatible vscale_range attributes
+define void @caller_unrestricted_min__callee_restricted_min() vscale_range(1, 4) {
+; CHECK-LABEL: @caller_unrestricted_min__callee_restricted_min(
+; CHECK-NEXT:    call void @callee_restricted_min()
+; CHECK-NEXT:    ret void
+;
+  call void @callee_restricted_min()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_unrestricted_min' inlined into 'caller_restricted_min__callee_unrestricted_min'
+define void @caller_restricted_min__callee_unrestricted_min() vscale_range(2, 4) {
+; CHECK-LABEL: @caller_restricted_min__callee_unrestricted_min(
+; CHECK-NEXT:    ret void
+;
+  call void @callee_unrestricted_min()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_unbounded_max' inlined into 'caller_restricted_max__callee_unbounded_max'
+define void @caller_restricted_max__callee_unbounded_max() vscale_range(1, 8) {
+; CHECK-LABEL: @caller_restricted_max__callee_unbounded_max(
+; CHECK-NEXT:    ret void
+;
+  call void @callee_unbounded_max()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_restricted_max' is not inlined into 'caller_unbounded_max__callee_restricted_max': incompatible vscale_range attributes
+define void @caller_unbounded_max__callee_restricted_max() vscale_range(1, 0) {
+; CHECK-LABEL: @caller_unbounded_max__callee_restricted_max(
+; CHECK-NEXT:    call void @callee_restricted_max()
+; CHECK-NEXT:    ret void
+;
+  call void @callee_restricted_max()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_restricted_max' inlined into 'caller_restricted_max__callee_restricted_max'
+define void @caller_restricted_max__callee_restricted_max() vscale_range(1, 8) {
+; CHECK-LABEL: @caller_restricted_max__callee_restricted_max(
+; CHECK-NEXT:    ret void
+;
+  call void @callee_restricted_max()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_unrestricted_max' inlined into 'caller_restricted_max__callee_unrestricted_max'
+define void @caller_restricted_max__callee_unrestricted_max() vscale_range(1, 8) {
+; CHECK-LABEL: @caller_restricted_max__callee_unrestricted_max(
+; CHECK-NEXT:    ret void
+;
+  call void @callee_unrestricted_max()
+  ret void
+}
+
+; CHECK-REMARK: remark: {{.*}} 'callee_restricted_max' is not inlined into 'caller_unrestricted_max__callee_restricted_max': incompatible vscale_range attributes
+define void @caller_unrestricted_max__callee_restricted_max() vscale_range(1, 16) {
+; CHECK-LABEL: @caller_unrestricted_max__callee_restricted_max(
+; CHECK-NEXT:    call void @callee_restricted_max()
+; CHECK-NEXT:    ret void
+;
+  call void @callee_restricted_max()
+  ret void
+}