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

[Allen zhong via Phabricator via llvm-commits]

Allen marked 4 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2014
       return true;
+  if (Q.CxtI && match(V, m_VScale())) {
+    const Function *F = Q.CxtI->getFunction();
----------------
david-arm wrote:
> 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.
Done, thanks 


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1945
     // constant.
+    KnownBits Known = computeKnownBits(Op1, 0, &I);
+    unsigned MaxActiveBits = Known.countMaxActiveBits();
----------------
david-arm wrote:
> 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);
>     ...
>   }
apply your comment, thanks


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1950
+      Constant *Zero = ConstantInt::getNullValue(Ty);
+      return BinaryOperator::CreateAnd(Zero, Op1);
+    }
----------------
david-arm wrote:
> I think you can avoid the `and` by simply returning null, i.e.
> 
>   return ConstantInt::getNullValue(Ty);
there is compilation problem if I **return ConstantInt::getNullValue(Ty)** directly
   error: cannot convert 'llvm::Constant*' to 'llvm::Instruction*' in return


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

https://reviews.llvm.org/D154953