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

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added inline comments.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2743
     // could still potentially be SubAdd.
     --E;
   }
----------------
efriedma wrote:
> Is there some reason to expect that --E is a valid, given E could be the first instruction in its basic block?
Good catch.  I'll guard this.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2750
   --I;
   for (; I != E; --I) {
     const MachineInstr &Instr = *I;
----------------
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.


https://reviews.llvm.org/D42263