[PATCH] D89703: [SVE][ValueTracking] Clarify TypeSize comparisons in llvm/lib/Transforms

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Thank you Sander and David.
I've changed the points you've raised. 
I had to run some test to make sure no scalable vectors would reach the places you've raised.
We can relax, atm no scalable vectors can reach the paths where we are using getFixedSize().



================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:1883
           return DT.dominates(S, L) &&
-                 DL.getTypeStoreSize(LTy) <= DL.getTypeStoreSize(STy);
+                 TypeSize::isKnownLE(DL.getTypeStoreSize(LTy),
+                                     DL.getTypeStoreSize(STy));
----------------
sdesmalen wrote:
> Globals are not allowed to be scalable, so this shouldn't use isKnownLE.
Thank you Sander. 



================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:899
   // it allows better simplification of the +1.
-  if (DL->getTypeSizeInBits(BECount->getType()) <
-          DL->getTypeSizeInBits(IntPtr) &&
+  if (TypeSize::isKnownLT(DL->getTypeSizeInBits(BECount->getType()),
+                          DL->getTypeSizeInBits(IntPtr)) &&
----------------
sdesmalen wrote:
> Does this need to work for scalable types?
After running some tests  I can confirm this path will never be accessible by a scalable vector. 
The test inside  Transforms/LoopIdiom/memcpy-vectors.ll confirms that this patch will never be reached.




================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:378
   if (isKnownNonNegative(LHS, *DL, 0, AC, GEP, DT) &&
-      DL->getTypeSizeInBits(LHS->getType()) <
-          DL->getTypeSizeInBits(GEP->getOperand(I)->getType())) {
+      TypeSize::isKnownLT(
+          DL->getTypeSizeInBits(LHS->getType()),
----------------
sdesmalen wrote:
> This is a functional change that now allows mixing scalable and fixed-width types. Either that should not happen (and thus it shouldn't use isKnownLT) or it should have a test that tests the new behaviour.
No vector(fix-width or scalable) will go down this path 
The test:
 if (SE->isSCEVable(I->getType()) && isPotentiallyNaryReassociable(&*I))
inside the method:
bool NaryReassociatePass::doOneIteration(Function &F)
prevents these types to reach this path.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2023
         return RHS->getType()->isIntegerTy() && !LHS->getType()->isIntegerTy();
-      return RHS->getType()->getPrimitiveSizeInBits() <
-             LHS->getType()->getPrimitiveSizeInBits();
+      return TypeSize::isKnownLT(RHS->getType()->getPrimitiveSizeInBits(),
+                                 LHS->getType()->getPrimitiveSizeInBits());
----------------
david-arm wrote:
> I think it's safe to use getFixedSize() here instead of TypeSize::isKnownLT because we know from the checks just above that these types must be integer types.
Good catch @david-arm. 
Thank you for spending the time on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89703