[PATCH] D38832: [X86][SelectionDAG] Add support for simplifying demanded bits of target nodes. Use it to simplify demanded bits of CMOV

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few comments but nice to see this being pursued - not sure if this will need breaking down into incremental changes at commit time.

Also, rather random question - why is SimplifyDemandedBits inside TargetLowering instead of in SelectionDAG with KnownBits/SignBits handling? Was the original intention for targets to override SimplifyDemandedBits directly?



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:27268
+  switch (Opc) {
+  case X86ISD::CMOV: {
+    if (SimplifyDemandedBits(Op.getOperand(1), DemandedMask, Known, TLO,
----------------
Do we have test cases for all of these? 


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:27293
+          return TLO.CombineTo(Op, NewOp);
+      }
+      }
----------------
bracket indentation?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:27312
+        }
+      }
+    }
----------------
Any easy way to remove this code duplication for Op0 and Op1?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:27322
+  return TargetLowering::SimplifyDemandedBitsForTargetNode(Op, DemandedMask,
+                                                           Known, TLO, Depth);
+}
----------------
For the other 'ForTargetNode' style calls we don't do a 'drop through' to the base class - should we match that approach here?


https://reviews.llvm.org/D38832