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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added a comment.

This looks sensible to me. Extend the comment in that `--E;` above at line 2732. If @efriedma  or @rengolin  do not have further comments I believe this is ready to land.



================
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.
----------------
jgalenson wrote:
> 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.  
Ah! Right. I hadn't noticed the `override` keyword above. Sorry for the fuss.


================
Comment at: test/CodeGen/ARM/su-addsub-overflow.ll:124
+define void @extern_loop(i32 %n) local_unnamed_addr #0 {
+; Do not replace the compare around the clobbering call.
+; CHECK: add {{r[0-9]+}}, {{r[0-9]+}}, #1
----------------
I understand this is a "sanity check"-like test and your change does not include new code to avoid this problem (all the machinery was already there). Is this correct?


https://reviews.llvm.org/D38378