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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
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;
+    }
----------------
jgalenson wrote:
> 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.
Thanks for the clarification, I'd leave it as it was unless it is easy to add a test that proves that this is certainly better for some cases but maybe was some interaction with my change in D35192 (while it was in trunk).


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2900
 
+bool ARMBaseInstrInfo::shouldSink(const MachineInstr &MI) const {
+  // Do not sink MI if it might be used to optimize a redundant compare.
----------------
Sorry, I fail to see where are you using this new function.


https://reviews.llvm.org/D38378