[PATCH] D67021: [DAGCombiner] improve throughput of shift+logic+shift

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Thanks, that is better.
LG with some thoughts.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7207-7209
+/// If we have a shift-by-constant of a bitwise logic op that itself has a
+/// shift-by-constant operand, we may be able to convert that into 2 independent
+/// shifts followed by the logic op. This is a throughput improvement.
----------------
+ the shifts must be identical


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7217-7219
+  if (LogicOpcode != ISD::AND && LogicOpcode != ISD::OR &&
+      LogicOpcode != ISD::XOR)
+    return SDValue();
----------------
Is there some function already that has whitelist of such bitwise ops?
I suspect there are more candidates.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7240-7243
+    // Shift amount types do not have to match their operand type, so check that
+    // the constants are the same width.
+    if (ShiftAmtVal->getBitWidth() != C1Val.getBitWidth())
+      return false;
----------------
This looks suspicious to be honest.
Is there a case where that is so?
If not, can this be an assert until then?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7247
+    unsigned BitWidth = V.getScalarValueSizeInBits();
+    if ((*ShiftAmtVal + C1Val).getLimitedValue(BitWidth) >= BitWidth)
+      return false;
----------------
I'd think `bool APInt::ult(uint64_t RHS)` would be fine to use too


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67021/new/

https://reviews.llvm.org/D67021