[ARM] Cleanup part of ARMBaseInstrInfo::optimizeCompareInstr (NFCI).

[jgalenson]

Summary

As noted in another review, this loop is confusing. This commit cleans it up somewhat.

[jgalenson]

I considered using reverse_iterator here, but I couldn't get something that looked clean to me, since I had to convert I and E and compare against E and the beginning of the block. I also considered splitting this loop into two, one that searches for SubAdd and one that checks for CPSR reads/writes, but that also seemed more complicated. I'd be open to suggestions on how to improve this further, though.

[efriedma]

since I had to convert I and E and compare against E and the beginning of the block

You should be able to tell which of "I != B" or "I != E" is the important exit condition before you enter the loop. Either E and I are in the same block (so I != E will fail before I != B), or they're not in the same block (so I != E will always be true).

[jgalenson]

You should be able to tell which of "I != B" or "I != E" is the important exit condition before you enter the loop. Either E and I are in the same block (so I != E will fail before I != B), or they're not in the same block (so I != E will always be true).

Yes, that's exactly what I did, but the extra logic to do that comparison, convert them into a reverse iterator, and get the bounds conditions correct (we want to consider E and B, so that would be inclusive, and if we instead used rend() that would be exclusive) seemed more complicated to me.

[efriedma]

lib/Target/ARM/ARMBaseInstrInfo.cpp

2754 ↗

(On Diff #130674)

Maybe it would be more clear to write "if (I == E) break;" before this check?

[jgalenson]

lib/Target/ARM/ARMBaseInstrInfo.cpp

2754 ↗

(On Diff #130674)

Sure, I can do that, but it does re-introduce an early exit into the loop, which I remember you didn't like before.

This, by the way, is exactly why I considered splitting this into two loops, which is another possibility, although it seemed a bit more complicated (and less efficient) to me.

[efriedma]

lib/Target/ARM/ARMBaseInstrInfo.cpp

2754 ↗

(On Diff #130674)

An early break isn't always ideal, but I think it makes sense here; it has a similar meaning to the other break in the if (isRedundantFlagInstr... block.

[efriedma]

LGTM

llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp

if (MI && IsThumb1) {
     }
     if (HasStmts && CanReorder) {
       MI = MI->removeFromParent();
       E = CmpInstr;
       CmpInstr.getParent()->insert(E, MI);
     }
     I = CmpInstr;
     E = MI;
-  } else if (E != B) {
-    // Allow the loop below to search E (which was initially MI).  Since MI and
-    // SubAdd have different tests, even if that instruction could not be MI, it
-    // could still potentially be SubAdd.
-    --E;
   }
 
   // Check that CPSR isn't set between the comparison instruction and the one we
   // want to change. At the same time, search for SubAdd.
   const TargetRegisterInfo *TRI = &getRegisterInfo();
-  --I;
-  for (; I != E; --I) {
-    const MachineInstr &Instr = *I;
+  do {
+    const MachineInstr &Instr = *--I;
 
     // Check whether CmpInstr can be made redundant by the current instruction.
-    if (isRedundantFlagInstr(&CmpInstr, SrcReg, SrcReg2, CmpValue, &*I)) {
+    if (isRedundantFlagInstr(&CmpInstr, SrcReg, SrcReg2, CmpValue, &Instr)) {
       SubAdd = &*I;
       break;
     }
 
+    // Allow E (which was initially MI) to be SubAdd but do not search before E.
+    if (I == E)
+      break;
+
     if (Instr.modifiesRegister(ARM::CPSR, TRI) ||
         Instr.readsRegister(ARM::CPSR, TRI))
       // This instruction modifies or uses CPSR after the one we want to
       // change. We can't do this transformation.
       return false;
 
-    if (I == B)
-      break;
-  }
+  } while (I != B);
 
   // Return false if no candidates exist.
   if (!MI && !SubAdd)
     return false;
 
   // The single candidate is called MI.
   if (!MI) MI = SubAdd;