[llvm] de18fa9 - Revert "[InstSimplify] Bypass no-op `and`-mask, using known bits (PR49543)"

Wed Apr 21 10:56:53 PDT 2021

Roman,

I don't see any evidence of bias against you here.  This patch was in 
tree for less than 10 hours, has substantial review comments raised, and 
was submitted without pre-commit review. Reverting seems entirely normal 
to me here.

On 4/21/21 9:21 AM, Roman Lebedev wrote:
> 1. I don't recall getting a message "hey, this is causing xyz, any
> thoughts, should revert?"
We don't require this, particularly for changes which have only been in 
tree a short time.  It's encouraged, but not required.
> 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
Can you point to specific wording you think is out of sync here? I'd 
like to tweak it if needed to remove the ambiguity.
>
> 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 :)

"isn't healthy" is strong wording here.  I hope you meant something more 
like "could be improved", and will respond as if you had.

I agree that we could probably improve our documented policies here.  
 From experience though, I will warn that there will *always* be a grey 
area in the middle which requires collaboration and discussion.  I've 
been the person trying to draft a good policy (for an employer) and it 
turns out to be nearly impossible to get 100% right all the time.

>
> 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 automation and documentation are great ideas.  Any chance you're 
willing to help make that happen?
>
> I think, next time my commit is reverted with points 1 and 2 not being met
> i'm going to recommit it.
That's certainly your choice.  I'll also note that your wording here 
misses the key "discuss and resolve concerns" step.
>
>
> 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