[PATCH] D18318: [AArch64][CodeGen] Fix of incorrect peephole optimization in AArch64InstrInfo::optimizeCompareInstr + peephole rules for Cmp+Brc

[Evgeny Astigeevich via llvm-commits]

eastig created this revision.
eastig added a subscriber: llvm-commits.
Herald added subscribers: rengolin, aemerson.

AArch64 peephole optimizer tries to remove a compare instruction. One case is comparison with 0: CmpInst %vreg, 0. It removes CmpInst and modifies the instruction defining ‘%vreg’ to the S version.  It is not checked that the S version and CmpInst produce the same condition flags.
For example the following code
```
%vreg7<def> = ANDWri %vreg6<kill>, 15; GPR32sp:%vreg7 GPR32:%vreg6
%vreg8<def> = SUBSWri %vreg7<kill>, 0, 0, %NZCV<imp-def>; GPR32:%vreg8 GPR32sp:%vreg7
Bcc 3, <BB#2>, %NZCV<imp-use>
```
is transformed to the incorrect code:
```
%vreg7<def> = ANDSWri %vreg6<kill>, 15, %NZCV<imp-def>; GPR32common:%vreg7 GPR32:%vreg6
Bcc 3, <BB#2>, %NZCV<imp-use>
```

It's incorrect because 'SUBSWri %vreg7<kill>, 0' always sets the carry flag to 1 but 'ANDSWri %vreg6<kill>, 15' always sets the carry flag to 0. As a result Bcc which checks the carry flag is not working as expected.

The submitted patch contains the fix of the defect and additional peephole rules for Cmp+Brc sequences. New regression tests are created.

**Changes description:**

•	Refactoring:

a.	Function name ‘modifiesConditionCode’ is changed to ‘areCFlagsAccessedBetweenInstrs’ to reflect that the function can check modifying accesses, reading accesses or both.
b.	Function ‘AArch64InstrInfo::optimizeCompareInstr’
        i. Documented the function
        ii.Cmp_NZCV  is DeadNZCVIdx to reflect that it is an operand index of dead NZCV
        iii. The code for the case of substituting CmpInstr is put into separate functions the main of them is ‘substituteCmpInstr’.

•	Fix of the defect:

A new algorithm was implemented:
1.	It’s checked that the same condition code is read by instructions after CmpInstr and before the next modification of NZCV. The optimization is not applied if different condition codes are used. It might be difficult to find a candidate for substitution to satisfy all of them. I think this case with multiple used condition codes does not happen often.
2.	Then it’s checked that the instruction which defines a register for CmpInstr can produce the needed condition code itself or its S variant. If it or its S variant can produce then CmpInstr is removed.

•	New peephole rules for Cmp+Brc:

The following rules were implemented: 

SUBS reg, 0; B.LO         => B <false path>
SUBS reg, 0; B.HS <label> => B <label>

•	Testing status: all existing + new tests passed.


http://reviews.llvm.org/D18318

Files:
  lib/Target/AArch64/AArch64InstrInfo.cpp
  lib/Target/AArch64/AArch64InstrInfo.h
  test/CodeGen/AArch64/cmp-bcc-opt.ll
  test/CodeGen/AArch64/subs-to-sub-opt.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D18318.51171.patch
Type: text/x-patch
Size: 25374 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160321/a6c88937/attachment.bin>