[X86]Remove TEST in AND32ri+TEST16rr in peephole-opt

[XinWang10]

Summary

Previously we remove a pattern like:

%reg = and32ri %in_reg, 5
...                         // EFLAGS not changed.
%src_reg = subreg_to_reg 0, %reg, %subreg.sub_index
test64rr %src_reg, %src_reg, implicit-def $eflags

We can remove test64rr since it has same functionality as and subreg_to_reg avoid the opt in previous code, so we handle this case specially.
And this case is also can be opted for the same reason, like:

%reg = and32ri %in_reg, 5
...                         // EFLAGS not changed.
%src_reg = copy %reg.sub_16bit:gr32
test16rr %src_reg, %src_reg, implicit-def $eflags

The COPY from gr32 to gr16 prevent the opt in previous code too, just handle it specially as what we did for test64rr.

[pengfei]

llvm/lib/Target/X86/X86InstrInfo.cpp

985–986

Can they be merged to one line?

llvm/test/CodeGen/X86/peephole-test-after-add.mir

41

Maybe pre-commit the test?

[craig.topper]

If the AND immediate is more than 16 bits, I don't think this transform is valid. Do we have a test for that?

[XinWang10]

In D154193#4464331, @craig.topper wrote:

If the AND immediate is more than 16 bits, I don't think this transform is valid. Do we have a test for that?

Good point! Will update.

[XinWang10]

llvm/lib/Target/X86/X86InstrInfo.cpp

985–986

Then the merged line would exceed 80 chars.

llvm/test/CodeGen/X86/peephole-test-after-add.mir

41

Will do, to make the change clear.

[pengfei]

llvm/lib/Target/X86/X86InstrInfo.cpp

985–986

Then you can put the exceeded part in the next line.

[XinWang10]

• Make sure instr is andri and limit imm for it

[XinWang10]

llvm/test/CodeGen/X86/peephole-test-after-add.mir

41

Done in https://reviews.llvm.org/D154322.

[XinWang10]

rebase

[craig.topper]

llvm/lib/Target/X86/X86InstrInfo.cpp

1007

Doesn't it need to be isUint<16>?

Bit 15 would also need to be 0 if the sign flag is being used.

[XinWang10]

use isUint instead

llvm/lib/Target/X86/X86InstrInfo.cpp

1007

Yes, It should be.

[skan]

I think you update the revision incorrectly...

[XinWang10]

update revision

[skan]

llvm/lib/Target/X86/X86InstrInfo.cpp

1000

Should this be an assert?

[XinWang10]

llvm/lib/Target/X86/X86InstrInfo.cpp

1000

No, VregDefInstr could be nullptr if CmpValDefInstr is just a copy from reg, and not a copy sub16bit from an AND32ri.

[XinWang10]

• update test

[XinWang10]

• update bigimm test because its imm is not 32bit

[skan]

llvm/test/CodeGen/X86/peephole-test-after-add.mir

57

Why change this?

[XinWang10]

llvm/test/CodeGen/X86/peephole-test-after-add.mir

57

The original intention is to test andri whose imm is not fit in 16bit, but the mir convert the 123456 to 57920, it offend the intention.

[skan]

llvm/test/CodeGen/X86/peephole-test-after-add.mir

57

Then remove %2, or reuse %2

[XinWang10]

• rm redundant var %store in bigimm test

[skan]

llvm/lib/Target/X86/X86InstrInfo.cpp

998

Only one check is needed

1011

Only one check is needed

[XinWang10]

• less the condition

[craig.topper]

llvm/lib/Target/X86/X86InstrInfo.cpp

1007

Do we have a test where we use the sign flag? The AND would set the sign flag based on bit 31. the TEST16rr would set the sign flag based on bit 15. So we could only do the transform if bit 31 and bit 15 are known to be zero (bit 31 is handled by the isUint<16> check. Or we need to not be using the sign flag from the test.

[XinWang10]

llvm/lib/Target/X86/X86InstrInfo.cpp

1007

I would add a test for it. It will be the same as the original logic for test64rr since the following code, if user use sf we will disable the removal.

[craig.topper]

llvm/lib/Target/X86/X86InstrInfo.cpp

1007

Ok I missed the NoSignFlag=true in the original code.

[XinWang10]

• add sign flag test for it

[XinWang10]

• restore unneed change

[skan]

LGTM

[chapuni]

This causes the crash in stage2 x86 clang. Could you investigate or revert?

[XinWang10]

In D154193#4484210, @chapuni wrote:

This causes the crash in stage2 x86 clang. Could you investigate or revert?

OK, I'd revert it first. Done in 284a059b33064764d5ebb1699b5ac8f6f5dad6e8, will invest it later.

[uabelho]

The following crashes with the patch

llc -march=x86-64 -o /dev/null reduced.ll

reduced.ll324 BDownload

[XinWang10]

In D154193#4484279, @uabelho wrote:

The following crashes with the patch

llc -march=x86-64 -o /dev/null reduced.ll

reduced.ll324 BDownload

Thanks a lot!

[uabelho]

In D154193#4484336, @XinWang10 wrote:

In D154193#4484279, @uabelho wrote:

The following crashes with the patch

llc -march=x86-64 -o /dev/null reduced.ll

reduced.ll324 BDownload

Thanks a lot!

Note: I found this with llvm-stress. I don't know if it's related to the build bot failure mentioned earlier here at all.

llvm/lib/Target/X86/X86InstrInfo.cpp

 }
 
 static bool findRedundantFlagInstr(MachineInstr &CmpInstr,
                                    MachineInstr &CmpValDefInstr,
                                    const MachineRegisterInfo *MRI,
                                    MachineInstr **AndInstr,
                                    const TargetRegisterInfo *TRI,
                                    bool &NoSignFlag, bool &ClearsOverflowFlag) {
-  if (CmpValDefInstr.getOpcode() != X86::SUBREG_TO_REG)
+  if (!(CmpValDefInstr.getOpcode() == X86::SUBREG_TO_REG &&
+        CmpInstr.getOpcode() == X86::TEST64rr) &&
+      !(CmpValDefInstr.getOpcode() == X86::COPY &&
+        CmpInstr.getOpcode() == X86::TEST16rr))
     return false;
 
-  if (CmpInstr.getOpcode() != X86::TEST64rr)
-    return false;
-
-  // CmpInstr is a TEST64rr instruction, and `X86InstrInfo::analyzeCompare`
-  // guarantees that it's analyzable only if two registers are identical.
-  assert(
-      (CmpInstr.getOperand(0).getReg() == CmpInstr.getOperand(1).getReg()) &&
-      "CmpInstr is an analyzable TEST64rr, and `X86InstrInfo::analyzeCompare` "
-      "requires two reg operands are the same.");
+  // CmpInstr is a TEST16rr/TEST64rr instruction, and
+  // `X86InstrInfo::analyzeCompare` guarantees that it's analyzable only if two
+  // registers are identical.
+  assert((CmpInstr.getOperand(0).getReg() == CmpInstr.getOperand(1).getReg()) &&
+         "CmpInstr is an analyzable TEST16rr/TEST64rr, and "
+         "`X86InstrInfo::analyzeCompare` requires two reg operands are the"
+         "same.");
 
   // Caller (`X86InstrInfo::optimizeCompareInstr`) guarantees that
   // `CmpValDefInstr` defines the value that's used by `CmpInstr`; in this case
   // if `CmpValDefInstr` sets the EFLAGS, it is likely that `CmpInstr` is
   // redundant.
   assert(
       (MRI->getVRegDef(CmpInstr.getOperand(0).getReg()) == &CmpValDefInstr) &&
-      "Caller guarantees that TEST64rr is a user of SUBREG_TO_REG.");
+      "Caller guarantees that TEST64rr is a user of SUBREG_TO_REG or TEST16rr "
+      "is a user of COPY sub16bit.");
+  MachineInstr *VregDefInstr = nullptr;
+  if (CmpInstr.getOpcode() == X86::TEST16rr) {
+    VregDefInstr = MRI->getVRegDef(CmpValDefInstr.getOperand(1).getReg());
+    if (!VregDefInstr)
+      return false;
+    // We can only remove test when AND32ri or AND64ri32 whose imm can fit 16bit
+    // size, others 32/64 bit ops would test higher bits which test16rr don't
+    // want to.
+    if (!((VregDefInstr->getOpcode() == X86::AND32ri ||
+           VregDefInstr->getOpcode() == X86::AND64ri32) &&
+          isUInt<16>(VregDefInstr->getOperand(2).getImm())))
+      return false;
+  }
 
-  // As seen in X86 td files, CmpValDefInstr.getOperand(1).getImm() is typically
-  // 0.
+  if (CmpInstr.getOpcode() == X86::TEST64rr) {
+    // As seen in X86 td files, CmpValDefInstr.getOperand(1).getImm() is
+    // typically 0.
     if (CmpValDefInstr.getOperand(1).getImm() != 0)
       return false;
 
     // As seen in X86 td files, CmpValDefInstr.getOperand(3) is typically
     // sub_32bit or sub_xmm.
     if (CmpValDefInstr.getOperand(3).getImm() != X86::sub_32bit)
       return false;
 
-  MachineInstr *VregDefInstr =
-      MRI->getVRegDef(CmpValDefInstr.getOperand(2).getReg());
+    VregDefInstr = MRI->getVRegDef(CmpValDefInstr.getOperand(2).getReg());
+  }
 
   assert(VregDefInstr && "Must have a definition (SSA)");
 
   // Requires `CmpValDefInstr` and `VregDefInstr` are from the same MBB
   // to simplify the subsequent analysis.
   //
   // FIXME: If `VregDefInstr->getParent()` is the only predecessor of
   // `CmpValDefInstr.getParent()`, this could be handled.
   if (VregDefInstr->getParent() != CmpValDefInstr.getParent())
     return false;
 
   if (X86::isAND(VregDefInstr->getOpcode())) {
     // Get a sequence of instructions like
     //   %reg = and* ...                    // Set EFLAGS
     //   ...                                // EFLAGS not changed
     //   %extended_reg = subreg_to_reg 0, %reg, %subreg.sub_32bit
     //   test64rr %extended_reg, %extended_reg, implicit-def $eflags
+    // or
+    //   %reg = and32* ...
+    //   ...                         // EFLAGS not changed.
+    //   %src_reg = copy %reg.sub_16bit:gr32
+    //   test16rr %src_reg, %src_reg, implicit-def $eflags
     //
     // If subsequent readers use a subset of bits that don't change
     // after `and*` instructions, it's likely that the test64rr could
     // be optimized away.
     for (const MachineInstr &Instr :
          make_range(std::next(MachineBasicBlock::iterator(VregDefInstr)),
                     MachineBasicBlock::iterator(CmpValDefInstr))) {
       // There are instructions between 'VregDefInstr' and
for (MachineInstr &Inst : make_range(From, MBB->rend())) {
       //     testl %eax, %eax   // <-- can be removed
       if (&Inst == SrcRegDef) {
         if (IsCmpZero &&
             isDefConvertible(Inst, NoSignFlag, ClearsOverflowFlag)) {
           MI = &Inst;
           break;
         }
 
-        // Look back for the following pattern, in which case the test64rr
-        // instruction could be erased.
+        // Look back for the following pattern, in which case the
+        // test16rr/test64rr instruction could be erased.
         //
-        // Example:
+        // Example for test16rr:
+        //  %reg = and32ri %in_reg, 5
+        //  ...                         // EFLAGS not changed.
+        //  %src_reg = copy %reg.sub_16bit:gr32
+        //  test16rr %src_reg, %src_reg, implicit-def $eflags
+        // Example for test64rr:
         //  %reg = and32ri %in_reg, 5
         //  ...                         // EFLAGS not changed.
         //  %src_reg = subreg_to_reg 0, %reg, %subreg.sub_index
         //  test64rr %src_reg, %src_reg, implicit-def $eflags
         MachineInstr *AndInstr = nullptr;
         if (IsCmpZero &&
             findRedundantFlagInstr(CmpInstr, Inst, MRI, &AndInstr, TRI,
                                    NoSignFlag, ClearsOverflowFlag)) {

llvm/test/CodeGen/X86/peephole-test-after-add.mir

define void @test_not_erased_when_eflags_change(ptr %0, i64 %1, i64 %2, i64 %3, ptr %4) {
     %8 = xor i64 %3, 5
     %9 = icmp eq i64 %7, 0
     %10 = select i1 %9, i64 %1, i64 %7
     store i64 %10, ptr %0, align 8
     store i64 %8, ptr %4, align 8
     ret void
   }
 
   define i16 @erase_test16(i16 %0, i16 %1, ptr nocapture %2) {
   entry:
     %3 = icmp ne i16 %0, 0
     %4 = and i16 %1, 123
     %5 = icmp eq i16 %4, 0
     %6 = select i1 %3, i1 %5, i1 false
     br i1 %6, label %if.then, label %if.end
 
   if.then:                                          ; preds = %entry
     store i16 %0, ptr %2, align 4
     br label %if.end
 
   if.end:                                           ; preds = %if.then, %entry
     ret i16 0
   }
 
   define i16 @erase_test16_bigimm(i16 %0, i32 %1, ptr nocapture %2) {
   entry:
     %3 = icmp ne i16 %0, 0
     %4 = and i32 %1, 123456
-    %truc = trunc i32 %4 to i16
-    %5 = icmp eq i16 %truc, 0
+    %trunc = trunc i32 %4 to i16
+    %5 = icmp eq i16 %trunc, 0
     %6 = select i1 %3, i1 %5, i1 false
     br i1 %6, label %if.then, label %if.end
 
   if.then:                                          ; preds = %entry
-    store i16 %0, ptr %2, align 4
+    store i32 %4, ptr %2, align 4
     br label %if.end
 
   if.end:                                           ; preds = %if.then, %entry
     ret i16 0
   }
 ...
 ---
 name:            test_erased
body:             |
   ; CHECK-NEXT:   [[COPY3:%[0-9]+]]:gr16 = COPY [[COPY2]].sub_16bit
   ; CHECK-NEXT:   TEST16rr [[COPY3]], [[COPY3]], implicit-def $eflags
   ; CHECK-NEXT:   JCC_1 %bb.3, 4, implicit $eflags
   ; CHECK-NEXT:   JMP_1 %bb.1
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1.entry:
   ; CHECK-NEXT:   successors: %bb.2(0x55555555), %bb.3(0x2aaaaaab)
   ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   [[AND32ri:%[0-9]+]]:gr32 = AND32ri [[COPY1]], 123, implicit-def dead $eflags
+  ; CHECK-NEXT:   [[AND32ri:%[0-9]+]]:gr32 = AND32ri [[COPY1]], 123, implicit-def $eflags
   ; CHECK-NEXT:   [[COPY4:%[0-9]+]]:gr16 = COPY [[AND32ri]].sub_16bit
-  ; CHECK-NEXT:   TEST16rr [[COPY4]], [[COPY4]], implicit-def $eflags
   ; CHECK-NEXT:   JCC_1 %bb.3, 5, implicit $eflags
   ; CHECK-NEXT:   JMP_1 %bb.2
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2.if.then:
   ; CHECK-NEXT:   successors: %bb.3(0x80000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   MOV16mr [[COPY]], 1, $noreg, 0, $noreg, [[COPY3]] :: (store (s16) into %ir.2, align 4)
   ; CHECK-NEXT: {{  $}}

 hasEHFunclets:   false
 isOutlined:      false
 debugInstrRef:   true
 failsVerification: false
 tracksDebugUserValues: false
 registers:
   - { id: 0, class: gr32, preferred-register: '' }
   - { id: 1, class: gr32, preferred-register: '' }
-  - { id: 2, class: gr64, preferred-register: '' }
-  - { id: 3, class: gr16, preferred-register: '' }
+  - { id: 2, class: gr32, preferred-register: '' }
+  - { id: 3, class: gr64, preferred-register: '' }
   - { id: 4, class: gr16, preferred-register: '' }
-  - { id: 5, class: gr32, preferred-register: '' }
+  - { id: 5, class: gr16, preferred-register: '' }
   - { id: 6, class: gr32, preferred-register: '' }
   - { id: 7, class: gr16, preferred-register: '' }
 liveins:
-  - { reg: '$edi', virtual-reg: '%0' }
-  - { reg: '$esi', virtual-reg: '%1' }
-  - { reg: '$rdx', virtual-reg: '%2' }
+  - { reg: '$edi', virtual-reg: '%1' }
+  - { reg: '$esi', virtual-reg: '%2' }
+  - { reg: '$rdx', virtual-reg: '%3' }
 frameInfo:
   isFrameAddressTaken: false
   isReturnAddressTaken: false
   hasStackMap:     false
   hasPatchPoint:   false
   stackSize:       0
   offsetAdjustment: 0
   maxAlignment:    1
body:             |
   ; CHECK-NEXT:   [[COPY3:%[0-9]+]]:gr16 = COPY [[COPY2]].sub_16bit
   ; CHECK-NEXT:   TEST16rr [[COPY3]], [[COPY3]], implicit-def $eflags
   ; CHECK-NEXT:   JCC_1 %bb.3, 4, implicit $eflags
   ; CHECK-NEXT:   JMP_1 %bb.1
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1.entry:
   ; CHECK-NEXT:   successors: %bb.2(0x55555555), %bb.3(0x2aaaaaab)
   ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   [[AND32ri:%[0-9]+]]:gr32 = AND32ri [[COPY1]], 57920, implicit-def dead $eflags
+  ; CHECK-NEXT:   [[AND32ri:%[0-9]+]]:gr32 = AND32ri [[COPY1]], 123456, implicit-def dead $eflags
   ; CHECK-NEXT:   [[COPY4:%[0-9]+]]:gr16 = COPY [[AND32ri]].sub_16bit
   ; CHECK-NEXT:   TEST16rr [[COPY4]], [[COPY4]], implicit-def $eflags
   ; CHECK-NEXT:   JCC_1 %bb.3, 5, implicit $eflags
   ; CHECK-NEXT:   JMP_1 %bb.2
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2.if.then:
   ; CHECK-NEXT:   successors: %bb.3(0x80000000)
   ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   MOV16mr [[COPY]], 1, $noreg, 0, $noreg, [[COPY3]] :: (store (s16) into %ir.2, align 4)
+  ; CHECK-NEXT:   MOV32mr [[COPY]], 1, $noreg, 0, $noreg, [[AND32ri]] :: (store (s32) into %ir.2)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.3.if.end:
   ; CHECK-NEXT:   [[MOV32r0_:%[0-9]+]]:gr32 = MOV32r0 implicit-def dead $eflags
   ; CHECK-NEXT:   [[COPY5:%[0-9]+]]:gr16 = COPY [[MOV32r0_]].sub_16bit
   ; CHECK-NEXT:   $ax = COPY [[COPY5]]
   ; CHECK-NEXT:   RET 0, $ax
   bb.0.entry:
     successors: %bb.3(0x60000000), %bb.2(0x20000000)
     liveins: $edi, $esi, $rdx
 
-    %2:gr64 = COPY $rdx
-    %1:gr32 = COPY $esi
-    %0:gr32 = COPY $edi
-    %3:gr16 = COPY %0.sub_16bit
-    TEST16rr %3, %3, implicit-def $eflags
+    %3:gr64 = COPY $rdx
+    %2:gr32 = COPY $esi
+    %1:gr32 = COPY $edi
+    %5:gr16 = COPY %1.sub_16bit
+    TEST16rr %5, %5, implicit-def $eflags
     JCC_1 %bb.2, 4, implicit $eflags
     JMP_1 %bb.3
 
   bb.3.entry:
     successors: %bb.1(0x55555555), %bb.2(0x2aaaaaab)
 
-    %5:gr32 = AND32ri %1, 57920, implicit-def dead $eflags
-    %4:gr16 = COPY %5.sub_16bit
+    %0:gr32 = AND32ri %2, 123456, implicit-def dead $eflags
+    %4:gr16 = COPY %0.sub_16bit
     TEST16rr %4, %4, implicit-def $eflags
     JCC_1 %bb.2, 5, implicit $eflags
     JMP_1 %bb.1
 
   bb.1.if.then:
     successors: %bb.2(0x80000000)
 
-    MOV16mr %2, 1, $noreg, 0, $noreg, %3 :: (store (s16) into %ir.2, align 4)
+    MOV32mr %3, 1, $noreg, 0, $noreg, %0 :: (store (s32) into %ir.2)
 
   bb.2.if.end:
     %6:gr32 = MOV32r0 implicit-def dead $eflags
     %7:gr16 = COPY %6.sub_16bit
     $ax = COPY %7
     RET 0, $ax
 
 ...