[PATCH] D37534: Unsigned saturation subtraction canonicalization [the backend part]

[Zvi Rackover via Phabricator via llvm-commits]

zvi added a comment.

Please add some details about the combine in the Summary. It's better to put a description in addition to the links to make it easier to read the commit log.
Maybe something like:
Combine:
 umax(a,b) - b  -> subus(a,b)
 a - umin(a,b)   -> subus(a,b)

And better add an X86 label to the title.



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:35489
+                                 const X86Subtarget &Subtarget) {
+  // TODO: Need to add IR cannonicialization for this code.
+  SDValue Op0 = N->getOperand(0);
----------------
Would a better place for this comment be right after the '// Try to find ... ' comment?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:35508
+    SDValue MaxRHS = Op0.getOperand(1);
+    if (DAG.isEqualTo(MaxLHS, Op1)) {
+      SubusLHS = MaxRHS;
----------------
Please drop braces for if/else with a single statement


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:35538
+  // if the value was zero extended from 16 bit.
+  if (SubusLHS.getOpcode() != ISD::ZERO_EXTEND) {
+    return SDValue();
----------------
Can we generalize by using DAG.computeKnownBits to tells us that the upper bits are zeroed?


Repository:
  rL LLVM

https://reviews.llvm.org/D37534