1. I don't recall getting a message "hey, this is causing xyz, any
thoughts, should revert?"
2. I don't recall seeing a follow-up mail to the original commit that
it was reverted
In fact, this isn't the first case i can recall.
Either that is so for my commits only, which means bias, or for all commits,
and then it (intentionally?) goes against wording about reverts on
https://llvm.org/docs/DeveloperPolicy.html /
https://llvm.org/docs/CodeReview.html
3. I think this current approach of dealing with compile-time
regressions isn't healthy. The heuristics aren't documented.
All of this is pretty much up to the reverter. As with every
niche stuff, the people involved day-to-day are usually biased
towards that niche stuff :)
I don't think this trend should continue.
I would suggest to avoid problems 1 and 2 in the future.
I would suggest to automatize point 1.
I would suggest to document the expectations.
I think, next time my commit is reverted with points 1 and 2 not being met
i'm going to recommit it.
Roman
On Wed, Apr 21, 2021 at 6:08 PM Philip Reames <listmail at philipreames.com> wrote:
>
> Roman,
>
> Not sure we have good docs on this, but the balancing act Nikita
> describes is definitely long standing practice.
>
> Philip
>
> On 4/21/21 1:00 AM, Roman Lebedev via llvm-commits wrote:
> > Could you please point me to the relevant quotes in the docs about compile time?
> >
> > On Wed, Apr 21, 2021 at 10:56 AM Nikita Popov via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Author: Nikita Popov
> >> Date: 2021-04-21T09:55:25+02:00
> >> New Revision: de18fa9e52a439798edf89df6fee807908814416
> >>
> >> URL: https://github.com/llvm/llvm-project/commit/de18fa9e52a439798edf89df6fee807908814416
> >> DIFF: https://github.com/llvm/llvm-project/commit/de18fa9e52a439798edf89df6fee807908814416.diff
> >>
> >> LOG: Revert "[InstSimplify] Bypass no-op `and`-mask, using known bits (PR49543)"
> >>
> >> This reverts commit ea1a0d7c9ae3e5232a4163fc67efad4aabd51f2b.
> >>
> >> While this is strictly more powerful, it is also strictly slower.
> >> InstSimplify intentionally does not perform many folds that it
> >> is allowed to perform, if doing so requires a KnownBits calculation
> >> that will be repeated in InstCombine.
> >>
> >> Maybe it's worthwhile to do this here, but that needs a more
> >> explicitly stated motivation, evaluated in a review.
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >> llvm/lib/Analysis/InstructionSimplify.cpp
> >> llvm/test/Transforms/InstSimplify/AndOrXor.ll
> >>
> >> Removed:
> >>
> >>
> >>
> >> ################################################################################
> >> diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
> >> index 43441184115f..08f504a0ce37 100644
> >> --- a/llvm/lib/Analysis/InstructionSimplify.cpp
> >> +++ b/llvm/lib/Analysis/InstructionSimplify.cpp
> >> @@ -2081,12 +2081,21 @@ static Value *SimplifyAndInst(Value *Op0, Value *Op1, const SimplifyQuery &Q,
> >> if (Value *V = simplifyLogicOfAddSub(Op0, Op1, Instruction::And))
> >> return V;
> >>
> >> - // A mask that only clears known zeros of a value is a no-op.
> >> + // A mask that only clears known zeros of a shifted value is a no-op.
> >> Value *X;
> >> const APInt *Mask;
> >> + const APInt *ShAmt;
> >> if (match(Op1, m_APInt(Mask))) {
> >> - KnownBits Known = computeKnownBits(Op0, Q.DL, 0, Q.AC, Q.CxtI, Q.DT);
> >> - if ((~*Mask).isSubsetOf(Known.Zero))
> >> + // If all bits in the inverted and shifted mask are clear:
> >> + // and (shl X, ShAmt), Mask --> shl X, ShAmt
> >> + if (match(Op0, m_Shl(m_Value(X), m_APInt(ShAmt))) &&
> >> + (~(*Mask)).lshr(*ShAmt).isNullValue())
> >> + return Op0;
> >> +
> >> + // If all bits in the inverted and shifted mask are clear:
> >> + // and (lshr X, ShAmt), Mask --> lshr X, ShAmt
> >> + if (match(Op0, m_LShr(m_Value(X), m_APInt(ShAmt))) &&
> >> + (~(*Mask)).shl(*ShAmt).isNullValue())
> >> return Op0;
> >> }
> >>
> >> @@ -2171,7 +2180,6 @@ static Value *SimplifyAndInst(Value *Op0, Value *Op1, const SimplifyQuery &Q,
> >> // SimplifyDemandedBits in InstCombine can optimize the general case.
> >> // This pattern aims to help other passes for a common case.
> >> Value *Y, *XShifted;
> >> - const APInt *ShAmt;
> >> if (match(Op1, m_APInt(Mask)) &&
> >> match(Op0, m_c_Or(m_CombineAnd(m_NUWShl(m_Value(X), m_APInt(ShAmt)),
> >> m_Value(XShifted)),
> >>
> >> diff --git a/llvm/test/Transforms/InstSimplify/AndOrXor.ll b/llvm/test/Transforms/InstSimplify/AndOrXor.ll
> >> index 09f6faf3928c..ee94d2f7720e 100644
> >> --- a/llvm/test/Transforms/InstSimplify/AndOrXor.ll
> >> +++ b/llvm/test/Transforms/InstSimplify/AndOrXor.ll
> >> @@ -1192,7 +1192,8 @@ define i8 @noop_and_t0(i8 %x) {
> >> ; CHECK-LABEL: @noop_and_t0(
> >> ; CHECK-NEXT: [[A:%.*]] = shl i8 [[X:%.*]], 3
> >> ; CHECK-NEXT: [[B:%.*]] = lshr i8 [[A]], 2
> >> -; CHECK-NEXT: ret i8 [[B]]
> >> +; CHECK-NEXT: [[R:%.*]] = and i8 [[B]], 62
> >> +; CHECK-NEXT: ret i8 [[R]]
> >> ;
> >> %a = shl i8 %x, 3
> >> %b = lshr i8 %a, 2
> >> @@ -1203,7 +1204,8 @@ define i8 @noop_and_t1(i8 %x) {
> >> ; CHECK-LABEL: @noop_and_t1(
> >> ; CHECK-NEXT: [[A:%.*]] = shl i8 [[X:%.*]], 3
> >> ; CHECK-NEXT: [[B:%.*]] = lshr i8 [[A]], 2
> >> -; CHECK-NEXT: ret i8 [[B]]
> >> +; CHECK-NEXT: [[R:%.*]] = and i8 [[B]], 126
> >> +; CHECK-NEXT: ret i8 [[R]]
> >> ;
> >> %a = shl i8 %x, 3
> >> %b = lshr i8 %a, 2
> >>
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits