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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

I think this is a really useful patch @Allen - thank you! It's definitely a step in the right direction. I do have a few comments on the patch ...



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2014
       return true;
+  if (Q.CxtI && match(V, m_VScale())) {
+    const Function *F = Q.CxtI->getFunction();
----------------
Perhaps this is better done in a separate patch? That way we can see what tests are affected by this change alone, since it is quite significant. Also, in the same patch you will need to update the LangRef to say that using the vscale_range attribute implies vscale is a power of 2.

Then, in a follow-on patch you can add the InstCombineMulDivRem.cpp change, which will show the tests that have changed purely due to the urem optimisation.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1945
     // constant.
+    KnownBits Known = computeKnownBits(Op1, 0, &I);
+    unsigned MaxActiveBits = Known.countMaxActiveBits();
----------------
I could be wrong, but I suspect this is a fairly expensive function to call. Perhaps it's worth restructuring the code to only call it when you know Op0 is a power of 2? For example, something like:

  if (match(Op0, m_Power2(RemC)) {
    KnownBits Known = computeKnownBits(Op1, 0, &I);
    ...
  }


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1950
+      Constant *Zero = ConstantInt::getNullValue(Ty);
+      return BinaryOperator::CreateAnd(Zero, Op1);
+    }
----------------
I think you can avoid the `and` by simply returning null, i.e.

  return ConstantInt::getNullValue(Ty);


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/rem-mul-shl.ll:39
+
+define i64 @urem_shl_vscale_out_of_range() #0 {
+; CHECK-LABEL: @urem_shl_vscale_out_of_range(
----------------
Based on your ValueTracking change I think you also need a negative test when vscale_range is set to something like (1,15)


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

https://reviews.llvm.org/D154953