[PATCH] D38378: [ARM] Optimize {s,u}{add,sub}.with.overflow.

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added inline comments.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2677-2679
   // One is MI, the other is a SUB instruction.
   // For CMPrr(r1,r2), we are looking for SUB(r1,r2) or SUB(r2,r1).
   // For CMPri(r1, CmpValue), we are looking for SUBri(r1, CmpValue).
----------------
rogfer01 wrote:
> Maybe I'm reading it wrong: should this comment say something like `the other is a SUB or ADD instruction`?
No, I'd missed that.  Thanks for pointing it out.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2741-2745
+    // Check whether CmpInstr can be made redundant by the current instruction.
+    if (isRedundantFlagInstr(&CmpInstr, SrcReg, SrcReg2, CmpValue, &*I)) {
+      SubAdd = &*I;
+      break;
+    }
----------------
rogfer01 wrote:
> It is not clear to me why you had to swap this check with the one below? Is it related to the `--E;` in line 2731?
Not really.  I believe the reason was that if the instruction I modifies CSPR, nothing before it can replace the compare, but it still can itself.  Swapping these two checks allowed that case.

However, when I undo this part of the change locally, my tests still pass.  I'm pretty sure I needed this before, but I guess something changed and I don't need it any longer.  So I can revert this piece if you'd like, although I still think the new way is better in theory.


https://reviews.llvm.org/D38378