[PATCH] D154953: [InstCombine] Remove the reminder 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 a comment.

Added new case in file llvm/test/Transforms/InstCombine/**AArch64**/rem-mul-shl.ll because it need a option -mtriple=aarch64-none-linux-gnu



================
Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:492
+  bool isVScaleKnownToBeAPowerOfTwo() {
+    return TTI.isVScaleKnownToBeAPowerOfTwo();
+  }
----------------
nikic wrote:
> This needs to be either a data layout property or a function attribute. Though it would probably best to change LangRef to require that vscale is //always// a power of two -- I think consensus has shifted towards non-pow2 vscales not being necessary.
according https://reviews.llvm.org/D141486, the document already clarify that the VScale will be known to be a power of 2


================
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)))) {
----------------
david-arm wrote:
> 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.
yes, apply your comment, thanks


================
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;
----------------
david-arm wrote:
> 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()))) {
Apply your comment, thanks


================
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);
----------------
david-arm wrote:
> This doesn't seem right - you're not using MaxPowerOf2 that you just calculated. Shouldn't this be:
> 
>   if (RemC->urem(MaxPowerOf2).isZero()) {
> 
> ?
Yes, you are right,thanks


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

https://reviews.llvm.org/D154953