[PATCH] D54959: [X86] Add a combine for back to back VSRAI instructions

Wed Nov 28 06:04:57 PST 2018

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

LGTM. Might want to leave a 'FIXME' note on that cost model test file. Or move anything of value in there that isn't already tested in the codegen test file.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:35471
+  if (Opcode == X86ISD::VSRAI && N0.getOpcode() == X86ISD::VSRAI &&
+      N0.hasOneUse()) {
+    unsigned ShiftVal2 = cast<ConstantSDNode>(N0.getOperand(1))->getZExtValue();
----------------
craig.topper wrote:
> spatel wrote:
> > I don't think we should limit this with hasOneUse (we don't have that restriction in the equivalent generic DAG or IR folds). Add a test for that scenario?
> Without it we generated extra moves on some other test cases in vector-shift-ashr-sub128.ll at least on pre-AVX targets.
Thanks - those diffs are what I imagined. It's better to trade a move for a shift (at least on paper) and reduce the dependency chain, and that's consistent with other folds.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54959