[PATCH] D12833: Combines pair of rotr/rotl with constant shifts into one instruction with combined shift

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A couple of minors but otherwise I think you're almost there.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5286
+  if (SDNode *C1 = DAG.isConstantIntBuildVectorOrConstantInt(N1))
+    if (NextOp == ISD::ROTL || NextOp == ISD::ROTR)
+      if (SDNode *C2 =
----------------
Might be worth testing this first - isConstantIntBuildVectorOrConstantInt calls are more costly so we can reduce wasted cpu cycles.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5295
+        SDValue BitsizeC = DAG.getConstant(Bitsize, dl, VT);
+        if (CombinedShift && BitsizeC) {
+          SDValue CombinedShiftNorm = DAG.FoldConstantArithmetic(
----------------
No need to test for BitsizeC - move the DAG.getConstant call inside "if (CombinedShift)"


================
Comment at: test/CodeGen/X86/combine-rotates.ll:62
 ; XOP:       # BB#0:
-; XOP-NEXT:    vprotd $31, %xmm0, %xmm0
-; XOP-NEXT:    vprotd $1, %xmm0, %xmm0
+; XOP-NEXT:    vprotd $0, %xmm0, %xmm0
 ; XOP-NEXT:    retq
----------------
Looks like we need a followup patch to remove rotates by zero.


https://reviews.llvm.org/D12833