[PATCH] D154953: [InstCombine] Remove the reminder loop if we know the mask is always true
David Sherwood via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 11 05:56:27 PDT 2023
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
More information about the llvm-commits
mailing list