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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111403