[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: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:
> 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).
Sure, I'll revert this part of the change.


================
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.
----------------
rogfer01 wrote:
> Sorry, I fail to see where are you using this new function.
I'm not calling it anywhere, but it's overriding a virtual method method in TargetInstrInfo.h that controls whether or not MachineSink should sink the given instruction.  


https://reviews.llvm.org/D38378