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

[James Molloy via llvm-commits]

jmolloy added a subscriber: jmolloy.
jmolloy requested changes to this revision.
jmolloy added a reviewer: jmolloy.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Evgeny,

Thanks for doing this. The documentation and cleanup is especially welcome.

I have comments below, but the main comment is that this needs to be split into two separate patches. One that performs a NFC refactoring, and another that actually fixes your bug. As is, it's very difficult for me to validate that the functional changes you've introduced are all required and what exactly they do differently.

Cheers,

James


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:825
@@ +824,3 @@
+  // From must be above To.
+  assert(std::find_if(MachineBasicBlock::reverse_iterator(To),
+                   To->getParent()->rend(),
----------------
Can you not just use

    std::find(MachineBasicBlock::reverse_iterator(To), To->getParent()->rend(), *From) ?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:834
@@ -813,5 +833,3 @@
 
-    if (Instr.modifiesRegister(AArch64::NZCV, TRI) ||
-        (!CheckOnlyCCWrites && Instr.readsRegister(AArch64::NZCV, TRI)))
-      // This instruction modifies or uses NZCV after the one we want to
-      // change.
+    if ((isAccess(AccessToCheck, AK_Write) && Instr.modifiesRegister(AArch64::NZCV, TRI)) ||
+        (isAccess(AccessToCheck, AK_Read)  && Instr.readsRegister(AArch64::NZCV, TRI)))
----------------
I think inlining the isAccess call in here would make things easier to read:

   if ( (AccessToCheck & AK_Write) && ...)

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:854
@@ +853,3 @@
+  assert(Instr);
+  MachineBasicBlock::iterator BeginInstrI = Instr;
+  ++BeginInstrI;
----------------
This could be better written as :

  auto BeginInstrI = std::next(Instr->getIterator());



================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:871
@@ +870,3 @@
+    case AArch64::Bcc:
+    {
+      int Idx = Instr.findRegisterUseOperandIdx(AArch64::NZCV);
----------------
Braces go on the same line as the case.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:887
@@ +886,3 @@
+    case AArch64::FCSELDrrr:
+    {
+      int Idx = Instr.findRegisterUseOperandIdx(AArch64::NZCV);
----------------
Braces go on the same line as the case.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:912
@@ +911,3 @@
+    case AArch64::SUBSXri:
+      return Instr.getOpcode();
+
----------------
The formatting of this is too verbose. The original code had one LoC per case, you have three. Please reformat as the original.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:960
@@ +959,3 @@
+static bool canInstrProduceCondCode(MachineInstr &Instr,
+        AArch64CC::CondCode NeededCondCode) {
+  switch (NeededCondCode) {
----------------
Not properly indented here

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:969
@@ +968,3 @@
+          break;
+
+        case AArch64::ADDWri:
----------------
Unneeded line break here

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1013
@@ +1012,3 @@
+    case AArch64CC::PL:
+    {
+      unsigned MiSOpcode = sForm(*MI);
----------------
Brace should be on the same line as the case.


http://reviews.llvm.org/D18318