[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),
----------------
fzhinkin wrote:
> 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).
A shameful part of why "AddedComplexity didn't help" is that I applied it incorrectly. :)
Otherwise it seems to work, I'll add test cases for code like `void a(int x, int y, void z()) { if ((x >> 1) == y) z(); }` and will come back with the update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131786