[PATCH] D13757: [x86] promote 'add nsw' to a wider type to allow more combines

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:25702
@@ +25701,3 @@
+  // Don't make the 'add' bigger if there's no hope of combining it with some
+  // other 'add' or 'shl' instruction.
+  // TODO: It may be profitable to generate simpler LEA instructions in place
----------------
spatel wrote:
> zansari wrote:
> > How about also SUB instructions to catch the simple constant merging case, similar to the one in add_nsw_consts?
> A 'sub with constant' is always canonicalized to 'add with -constant':
>   // fold (sub x, c) -> (add x, -c)
> http://llvm.org/docs/doxygen/html/DAGCombiner_8cpp_source.html#l01904
> 
> So I was about to say that we don't have to worry about the 'sub' case...except that the above transform doesn't propagate the 'nsw'. So we still don't have to worry about the 'sub' case in this code. :)
> 
> I noted the lack of nsw propagation bug in D12095 (and Hal warned about the consequences), but I didn't do anything about it...and now it's crawled out from under the rug. Worse still, I didn't think about adding an 'nsw' to the wide 'add' that I'm creating in this patch. 
> 
> In my defense, getting the flags right in IR transforms has been an ongoing challenge, so at least we're conservatively correct by dropping the flags...I hope. cc'ing Sanjoy and David for their nsw expertise.
> 
> Unless there's a correctness bug here, I think we should get this patch checked in, and then we'll have to start working on propagating nsw/nuw/exact flags all over the DAG. 
It should be fine to transform `sext(add nsw %X, %Y)` into `add nsw (sext %X), (sext %Y)`.


http://reviews.llvm.org/D13757