[PATCH] D111403: [Inliner] Don't inline functions with incompatible vscale_range attributes
Cullen Rhodes via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 12 09:59:50 PDT 2021
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
More information about the llvm-commits
mailing list