[PATCH] D52463: [ARM] Fix for PR39060

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:249
     return true;
 
+  // We can support a, potentially, overflowing instruction if:
----------------
Thanks for commenting on this, this is very useful! A few nits below.


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:253
+  // - the icmp uses a constant.
+  // - the overflowing value is decreasing, i.e would underflow
+  // - the underflowing instruction also uses a constant.
----------------
can you specify what exactly the "overflowing value" in the pattern is?


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:254
+  // - the overflowing value is decreasing, i.e would underflow
+  // - the underflowing instruction also uses a constant.
+  //
----------------
and similarly, what the "underflowing instruction" is?


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:255
+  // - the underflowing instruction also uses a constant.
+  //
+  // We can then use the two constants to calculate whether the result would
----------------
Thinking out loud.... I think we could be a bit more precise. Something along the lines of:

We are pattern matching a sequence of 2 instructions:
- Instruction I is and add or sub, with a LHS, and a RHS that should be a constant C1, and
- User of I is a compare instruction CI:  the LHS is instruction I, the RHS a constant C2.

Instruction I needs to be decreasing, which is the case when instruction I as an add  and C1 is negative or when I is a sub and C1 positive,
because <insert reason here>. 

To determine whether wrapping occurs, we calculate a new constant C3:

  C3 = abs(C1) + C2,       where C1 and C2 are zero-extend to 32 bits if necessary.

We define a new constant:
  
  Max = ....

And overflow in C3 is okay if:
- ...
- ...


https://reviews.llvm.org/D52463