[PATCH] D131786: [ARM] Support all versions of AND, ORR, EOR and BIC in optimizeCompareInstr

[Filipp Zhinkin via Phabricator via llvm-commits]

fzhinkin added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrInfo.td:4888
+// as it will be handled separately after isel by the peephole
+// that fuse a shift with a comparison (by using shift's S-version).
 def : ARMPat<(ARMcmpZ GPR:$src, mod_imm:$imm),
----------------
efriedma wrote:
> I'm not sure how you use the shift's S-version in general.  Do we have test coverage for something like `void a(int x, int y, void z()) { if ((x >> 1) == y) z(); }`?
> 
> (If the issue is that we're choosing CMPrsi over CMPri, you can probably use AddedComplexity to force the preferred form.)
Thank you for taking a look at it! It seems like there are no tests covering this particular case (and I didn't add it either :( ), and for code that looks like something you mentioned removal of these patterns will result in  a worse code sequence (shift + cmp instead of single cmp). I'll roll back this part of the change.

The issue I tried to overcome by removing two ARMcmpZ patterns is following:
consider a code like `return (a >> b) != 0 ? (a >> b) : 42` (https://godbolt.org/z/EfsTq3dP4).
Before instruction selection corresponding DAG will looks like:
```
    t0: ch = EntryToken
    t2: i32,ch = CopyFromReg t0, Register:i32 %0
    t4: i32,ch = CopyFromReg t0, Register:i32 %1
  t5: i32 = srl t2, t4
      t16: glue = ARMISD::CMPZ t5, Constant:i32<0>
    t17: i32 = ARMISD::CMOV t5, Constant:i32<42>, Constant:i32<0>, Register:i32 $cpsr, t16
  t12: ch,glue = CopyToReg t0, Register:i32 $r0, t17
  t13: ch = ARMISD::RET_FLAG t12, Register:i32 $r0, t12:1
```

`CMPZ t5` where `t5` is `i32 = srl t2, t4` will match pattern for ARMcmpZ, so after instr selection we'll get:
```
 t0: ch = EntryToken
  t2: i32,ch = CopyFromReg t0, Register:i32 %0
  t4: i32,ch = CopyFromReg t0, Register:i32 %1
      t5: i32 = MOVsr t2, t4, TargetConstant:i32<3>, TargetConstant:i32<14>, Register:i32 $noreg, Register:i32 $noreg
        t6: i32 = MOVi TargetConstant:i32<0>, TargetConstant:i32<14>, Register:i32 $noreg, Register:i32 $noreg
      t16: i32,glue = CMPrsr t6, t2, t4, TargetConstant:i32<3>, TargetConstant:i32<14>, Register:i32 $noreg
    t17: i32 = MOVCCi t5, TargetConstant:i32<42>, TargetConstant:i32<0>, Register:i32 $cpsr, t16:1
  t12: ch,glue = CopyToReg t0, Register:i32 $r0, t17
  t13: ch = BX_RET TargetConstant:i32<14>, Register:i32 $noreg, Register:i32 $r0, t12, t12:1
```

`CMPrsr` (`t16`) is no longer related to `t5` and `isOptimizeCompareCandidate` won't be able to replace it with S-version of `t5`.


I tried to use AddedComplexity, but it didn't help.
Probably, the better place to handle this case is `ARMTargetLowering::getARMCmp` (and there is a TODO suggesting to do that there, and I missed it previously).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131786