[PATCH] D48789: [X86] Replace (32/64 - n) shift amounts with (neg n) since the shift amount is masked in hardware

[Nirav Dave via Phabricator via llvm-commits]

niravd added inline comments.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2737
+      insertDAGNode(*CurDAG, OrigShiftAmt, Zero);
+      insertDAGNode(*CurDAG, OrigShiftAmt, Neg);
+    } else
----------------
There's a definitely an issue with insertDAGNode where collisions due to CSE could require a nontrivial reordering in AllNodes to maintain the 'users before operands' property and we do at most repositioning one node. At some point we should probably replace insertDAGNode with a backend-generic way to corectly reposition that node and it's operands recursively to guarantee correct behavior. 

That said, hitting the issue in a way that will mess with correctness is pretty hard.We should probably change the position given to insertDAGNode to be the immediate operand to minimize the probability of reordering size. 





================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2758
+  // Recapture operand 1 so we can delete it if its unused.
+  CurDAG->UpdateNodeOperands(N, N->getOperand(0), NewShiftAmt);
+
----------------
You should overwrite N with the return value of the update in case the update is CSEd with another node, e.g., we are selecting in a block that has both the pre-optimized and post-optimized versions. Actually, we should probably add a test case for this.

N = CurDAG->UpdateNodeOperands(N, N->getOperand(0), NewShiftAmt);


https://reviews.llvm.org/D48789