[PATCH] D154953: [InstCombine] Remove the reminder loop if we know the mask is always true

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @Allen, thanks for this patch - it's a nice IR optimisation!

Can you add some specific tests to llvm/test/Transforms/InstCombine/rem-mul-shl.ll as well for all the cases covered by your change?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1900
+  std::optional<unsigned> MaxVScale = getMaxVScale(*F);
+  if (MaxVScale && match(Op0, m_APInt(RemC)) &&
+      match(Op1, m_Shl(m_VScale(), m_APInt(ShiftC)))) {
----------------
You also need to call TTI.isKnownToBeAPowerOfTwo to check if vscale is a power of 2, i.e.

  if (MaxVScale && TTI.isKnownToBeAPowerOfTwo() && match(Op0, m_APInt(RemC)) &&
...

If vscale is a power of 2, then you can assume that if the remainder is 0 for the maximum value of vscale, it will also be 0 for any smaller value too.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1901
+  if (MaxVScale && match(Op0, m_APInt(RemC)) &&
+      match(Op1, m_Shl(m_VScale(), m_APInt(ShiftC)))) {
+    APInt MaxPowerOf2 = APInt(ShiftC->getBitWidth(), *MaxVScale) << *ShiftC;
----------------
I think you might also want to check if Op1 is just vscale:

  (match(Op1, m_Shl(m_VScale(), m_APInt(ShiftC))) ||
   match(Op1, m_VScale()))) {


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1902
+      match(Op1, m_Shl(m_VScale(), m_APInt(ShiftC)))) {
+    APInt MaxPowerOf2 = APInt(ShiftC->getBitWidth(), *MaxVScale) << *ShiftC;
+    if (RemC->urem(*ShiftC).isZero()) {
----------------
If you follow my suggestion above about also checking for just `urem RemC, vscale()` then you'll need to change this to:

  APInt MaxPowerOf2 = APInt(RemC->getBitWidth(), *MaxVScale) << (ShiftC ? *ShiftC : 1);

and initialise ShiftC to nullptr at the declaration.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1903
+    APInt MaxPowerOf2 = APInt(ShiftC->getBitWidth(), *MaxVScale) << *ShiftC;
+    if (RemC->urem(*ShiftC).isZero()) {
+      Constant *Zero = ConstantInt::get(I.getType(), 0);
----------------
This doesn't seem right - you're not using MaxPowerOf2 that you just calculated. Shouldn't this be:

  if (RemC->urem(MaxPowerOf2).isZero()) {

?


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/pr34438.ll:47
 ; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp eq i64 [[INDVARS_IV_NEXT]], 8
-; CHECK-NEXT:    br i1 [[EXITCOND]], label [[FOR_END]], label [[FOR_BODY]], !llvm.loop [[LOOP3:![0-9]+]]
+; CHECK-NEXT:    br i1 [[EXITCOND]], label [[FOR_END]], label [[FOR_BODY]], !llvm.loop [[LOOP4:![0-9]+]]
 ; CHECK:       for.end:
----------------
nit: Seems like an unnecessary change.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr44488-predication.ll:68
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i16 [[INC7]], 111
-; CHECK-NEXT:    br i1 [[CMP]], label [[FOR_BODY]], label [[EXIT]], !llvm.loop [[LOOP2:![0-9]+]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[FOR_BODY]], label [[EXIT]], !llvm.loop [[LOOP3:![0-9]+]]
 ; CHECK:       exit:
----------------
nit: Seems like an unnecessary change.


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

https://reviews.llvm.org/D154953