[PATCH] D87227: [SVE] Fix isLoadInvariantInLoop for scalable vectors

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfoImpl.h:818
+          // vscale and the target must be able to cope with that.
+          Scale = ElementSize.getKnownMinSize();
         }
----------------
sdesmalen wrote:
> It is probably important for `isLegalAddressingMode` to know whether the Offset and/or Scale is a multiple of vscale, so it makes sense to encode that in a TypeSize. I would prefer to either keep the warning or make isLegalAddressingMode accept a TypeSize, instead of explicitly removing the 'vscale' information here and replacing the warning with a TODO.
OK I'll make the change to isLegalAddressingMode in a separate patch to support compound BaseOffsets that are true polynomials, i.e. pass a BaseOffset and a ScaledBaseOffset to isLegalAddressingMode.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:988
+    // so we should check for that here.
+    if (InvariantSize->isMinusOne())
+      continue;
----------------
sdesmalen wrote:
> This may be better addressed in a separate patch, because this is unrelated to SVE or scalable vectors.
> 
> Just to confirm what's at fault here. If the number of invariant bits of an object is unknown, it is wrong to assume that the load is invariant by checking `LocSizeInBits.getFixedSize() <= InvariantSizeInBits`. Here `InvariantSizeInBits` would be `(unsigned) -1`, so this is always true. Where in fact, we expect it to evaluate to `false`.
So it is related to SVE in a way, because if the C++ frontend in clang ever did decide to support sizeless types in structs, classes or thread locals (SVE ACLE types are sizeless) then this value may actually be -1 (unless the definition of the intrinsic is changed to formally deal with scalable types).

Yes, the problem is that in the code below we are potentially casting -1 (which can occur for any incomplete type in clang) to unsigned and therefore always hoisting the load out, which is wrong. I think if we expect to never hit the -1 case here then there should probably be an assert instead.


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

https://reviews.llvm.org/D87227