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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen 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();
         }
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:954
+  // of the latter.
+  if (LocSizeInBits.isScalable())
+    return false;
----------------
This change seems untested, as none of your tests has a `llvm.invariant.start intrinsic` (and so there is no code-path tested that would otherwise return `true`)


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:988
+    // so we should check for that here.
+    if (InvariantSize->isMinusOne())
+      continue;
----------------
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`.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:990
+      continue;
+    assert(!InvariantSize->isNegative() &&
+           "Unknown invariant.start size value");
----------------
It seems odd to assert that here. This is probably better added as a check to the IR Verifier pass, and here bail out with:
```if (InvariantSize < 0)
  continue;```


================
Comment at: llvm/test/Transforms/LICM/AArch64/sve-load-hoist.ll:1
+; RUN: opt -licm -mtriple aarch64-linux-gnu -mattr=+sve -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
----------------
These tests don't test anything from the patch, as they already pass with current master.


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

https://reviews.llvm.org/D87227