[PATCH] D42263: [ARM] Fix perf regression in compare optimization.

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM with the comment fixed.



================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2750
   --I;
   for (; I != E; --I) {
     const MachineInstr &Instr = *I;
----------------
jgalenson wrote:
> efriedma wrote:
> > This loop is confusing.  The initial value of I is the compare.  You're looping until it equals E... but you also have an early exit if you hit the beginning of the basic block.
> > 
> > Could you fix the loop to explicitly compute its exit condition in some more intuitive way?  Maybe you could express this more clearly using MachineBasicBlock::reverse_iterator.  And ideally you could use range-based for loop with "make_range" rather an explicit increment/decrement.
> I agree that the loop is confusing, but I'd rather make that change as part of a separate commit, since doing so is surprisingly non-trivial and could well introduce new bugs.
Okay.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2766
     if (I == B)
       // The 'and' is below the comparison instruction.
+      break;
----------------
Please fix this comment; there is no 'and', and instructions after the comparison are irrelevant.


https://reviews.llvm.org/D42263