[PATCH] D74165: [x86] [DAGCombine] Prefer shifts of constant widths.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Have you found any other targets that would benefit from this? Otherwise we might consider put it inside X86ISelLowering initially?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:491
 
+    // SHL, SRA, SRL, RTOL, ROTR, but FSHL or FSHR.
+    SDValue visitShiftOrRotate(SDNode *N);
----------------
RKSimon wrote:
> "but not FHL or FSHR"? Why shouldn't funnels shifts be included?
OK, but maybe change this to a TODO for FSHL/FSHR ?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7306
+SDValue DAGCombiner::visitShiftOrRotate(SDNode *N) {
+  auto ShiftOpcode = N->getOpcode();
+  SDValue LHS = N->getOperand(0);
----------------
(style) Don't use auto.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7446
+  if (SDValue V = visitShiftOrRotate(N))
+    return V;
+
----------------
Move these lower down to after SimplifyDemandedBits, we tend to have the constant folding / cleanup combines first.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:46546
+  // shifts/rotates by a register.
+  auto Opcode = N->getOpcode();
+  assert(Opcode == ISD::SHL || Opcode == ISD::SRA || Opcode == ISD::SRL ||
----------------
(style) Don't use auto.


================
Comment at: llvm/test/CodeGen/X86/dagcombine-shifts.ll:225
+; in the result and that they take immediates rather than registers.
+define i32 @shl_select(i32 %x, i1 %cond) {
+; CHECK-LABEL: shl_select:
----------------
Please commit these tests with current codegen, then rebase the patch to show the codegen diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74165