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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1753-1767
+/// prevent inlining that could otherwise lead to invalid results. There are
+/// several cases to consider:
+///
+///   1. If neither the caller or callee have a vscale_range attribute, allow
+///      inlining.
+///   2. If either of the caller or callee have a vscale_range attribute, but
+///      not both, don't inline, since it may be unsafe to do so.
----------------
sdesmalen wrote:
> nit: the code below says as much, so the comment is a bit redundant?
> nit: the code below says as much, so the comment is a bit redundant?

I've removed the comment.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1770
+                                                  const Function &Callee) {
+  bool CallerHasVScaleRange = Caller.hasFnAttribute(Attribute::VScaleRange);
+  bool CalleeHasVScaleRange = Callee.hasFnAttribute(Attribute::VScaleRange);
----------------
sdesmalen wrote:
> 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.
> 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.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1800
+  // don't match.
+  if (!CallerMinVScale || !CallerMaxVScale || !CalleeMinVScale ||
+      !CalleeMaxVScale)
----------------
sdesmalen wrote:
> david-arm wrote:
> > sdesmalen wrote:
> > > 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.
> > 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.
> 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.
> 
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111403/new/

https://reviews.llvm.org/D111403