[PATCH] D124118: [Peephole-Opt] For one kind of test-after-add pattern, eliminates test if it's correct to do so.

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added a comment.

In D124118#3463778 <https://reviews.llvm.org/D124118#3463778>, @davidxl wrote:

> In D124118#3463462 <https://reviews.llvm.org/D124118#3463462>, @craig.topper wrote:
>
>> In D124118#3463205 <https://reviews.llvm.org/D124118#3463205>, @davidxl wrote:
>>
>>> I suspect there is something else going on.
>>>
>>> The following case works fine without the patch.
>>>
>>> define dso_local noundef i64 @_Z3fooPlll(ptr nocapture noundef %0, i64 noundef %1, i64 noundef %2) local_unnamed_addr #0 {
>>>
>>>   %4 = load i64, ptr %0, align 8
>>>   %5 = and i64 %4, 3
>>>   %6 = icmp eq i64 %5, 0
>>>   %7 = select i1 %6, i64 %1, i64 %2
>>>   store i64 %7, ptr %0, align 8
>>>   ret i64 %5
>>>
>>> }
>>>
>>> However a simple change below to replace the operand %2 in the select with %5 (output of and operation), then it stops working.
>>>
>>> define dso_local noundef i64 @_Z3fooPlll(ptr nocapture noundef %0, i64 noundef %1, i64 noundef %2) local_unnamed_addr #0 {
>>>
>>>   %4 = load i64, ptr %0, align 8
>>>   %5 = and i64 %4, 3
>>>   %6 = icmp eq i64 %5, 0
>>>   %7 = select i1 %6, i64 %1, i64 %5
>>>   store i64 %7, ptr %0, align 8
>>>   ret i64 %5
>>>
>>> }
>>>
>>> Can you investigate the difference?  The behavior difference on ARM is also worth comparing with.
>>
>> There's a peephole in X86DAGToDAGISel::Select and PostProcessISelDAG that both fail if the AND has users other than the TEST.
>
> thanks. That explains it.
>
> For this case, the subreg_to_reg is indeed the reason blocking the peephole-opt from cleaning it up (as can be seen in the case if the AND operand is changed to 0x1ffffffff where subreg_to_reg is not generated)

Besides peephole-opt in X86ISelDAGToDAG that looks for `(X86cmp (and $op, $imm), 0)`[1], another significance is in legalization

Using `llc -O3 -debug -debug-only=isel,x86-isel,selectiondag,legalizedag,peephole-opt,x86-instr-info <file.ll>` gives the following log respectively

- https://gist.github.com/minglotus-6/39fb318886ae8deaf34a6f2f0d03fceb for the good case (`TEST64rr` erased)
- https://gist.github.com/minglotus-6/fcd07bc00d9bc362b7bd5675ba131740 for the bad case  (`TEST64rr` kept)

To elaborate,

- for the good case
  - https://gist.github.com/minglotus-6/39fb318886ae8deaf34a6f2f0d03fceb#file-first-log-L96-L104 show legalization doesn't generate `CMP`.
- for the bad case
  - https://gist.github.com/minglotus-6/fcd07bc00d9bc362b7bd5675ba131740#file-second-log-L93-L101 shows legalization generates `CMP`

Tracking the implementation, X86TargetLowering::emitFlagsForSetcc <https://raw.githubusercontent.com/llvm/llvm-project/main/llvm/lib/Target/X86/X86ISelLowering.cpp> has different paths

- when `AND` has one use (around line 24366), no cmp is emit
- for the bad case, will call `EmitCmp` around line 24426

(X86ISelLowering.cpp is too large to display in Github, or generate a permanent link, so add the line number)

[1] https://github.com/llvm/llvm-project/blob/360d44e86defea94fb5608765fbdbfdb2a36f4c6/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp#L5608


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124118