[PATCH] D40922: [ARM] Optimize {s|u}mul.with.overflow.

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:3955
+  case ISD::SMULO:
+    // We generate a SMUL_LOHI and then check if the high word is 0 or
+    // 0xffffffff.
----------------
rogfer01 wrote:
> What about 
> 
> ```lang=cpp
> // We generate a SMUL_LOHI and then check if all the bits of the high word
> // are the same as the sign bit of the low word.
> ```
That's definitely better.  Thanks!


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4479
 
   // Optimize {s|u}{add|sub}.with.overflow feeding into a branch instruction.
   unsigned Opc = Cond.getOpcode();
----------------
rogfer01 wrote:
> I think this comment should be updated after this change.
Good catch!


================
Comment at: test/CodeGen/ARM/su-addsub-overflow.ll:80
 
+define i32 @smul(i32 %a, i32 %b) local_unnamed_addr #0 {
+; CHECK-LABEL: smul:
----------------
rogfer01 wrote:
> Looks like we are overloading a bit the name of this test file. Perhaps add a new testfile `su-mul-overflow.ll` instead?
Good point.  I think I prefer having them all in the same file since they're testing the same sort of things, so I'll rename the file.  But I can certainly split it if you think that's best.


https://reviews.llvm.org/D40922