[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: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.
----------------
nit: the code below says as much, so the comment is a bit redundant?


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


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1779
+  // both, don't allow inlining.
+  if (CallerHasVScaleRange != CalleeHasVScaleRange)
+    return true;
----------------
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)`


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111403