[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:2758
+  // Recapture operand 1 so we can delete it if its unused.
+  CurDAG->UpdateNodeOperands(N, N->getOperand(0), NewShiftAmt);
+
----------------
craig.topper wrote:
> niravd wrote:
> > 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);
> I'm having a hard time getting the topological sort to give me an order that can cause this to happen. We're always doing isel on the post-optimized version i explicitly put in the IR first. There are more nodes in that path and that seems to be making it so that the pre-optimized path gets fully sorted into the topological order before the post-optimized side does.
I don't have the details of the topological sort worked out, but it definitely seems reasonable that it precludes any problematic orderings. Given that, let's skip on adding a test case for this. 



https://reviews.llvm.org/D48789