[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 marked an inline comment as done.
mingmingl added a comment.

thanks for reviews!



================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:970
 
+// Returns true if SF is known to be zero after AndInstr executes.
+inline static bool isSFZero(const MachineInstr &AndInstr) {
----------------
craig.topper wrote:
> I don't know if this is necessary. If the SUBREG_TO_REG was used, the sign bit of the full 64 bit value was provably zero during SelectionDAG. I hope any branch, setcc, cmov would have been folded away based on it.
> 
> Can't we just set NoSignFlag=true?
`isSFZero` is used to tell the difference of following two scenarios, and (if I read correctly) `NoSignFlag` (as well as `ClearsOverflowFlag`) indicate whether the EFLAG bit is set (as opposed to the value of bit), so for `AND` operation, we want to set `NoSignFlag` to false.

1) 

```
AND32ri8 reg, 0x9  # signed bit is 1, so after sign extension, the SF bit could be 1 or 0 depending on reg value
SUBREG_TO_REG 0, extended_reg, reg , sub_32bit
TEST64rr extended_reg, extended_reg,  # SF bit is 0 since higher 32 bit of extended_reg is 0
```

2) 

```
AND32ri8 reg, 0x7  # signed bit is 0, so after sign extension, the SF bit is known to be 0
SUBREG_TO_REG 0, extended_reg, reg , sub_32bit
TEST64rr extended_reg, extended_reg,  # SF bit is 0 since higher 32 bit of extended_reg is 0
```

We want to optimize away `test` for 2) but keep `test` for 1).

Let me know if I miss anything!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124118