[PATCH] D41856: [PowerPC] Zero-extend the compare operand for ATOMIC_CMP_SWAP

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

In https://reviews.llvm.org/D41856#972501, @efriedma wrote:

> > Do you mean that the general approach of updating the nodes during custom legalization is fragile
>
> Yes.  The problem is that you're essentially attaching additional semantics to a target-independent opcode (specifically, that the "unused" bits of the ATOMIC_CMP_SWAP must be zero).  DAGCombine doesn't understand this, and can therefore break your assumptions.  This has come up before in other contexts (e.g. https://reviews.llvm.org/D31331).
>
> The clean approach is to introduce a target-specific node (e.g. PPCISD::ATOMIC_CMP_SWAP), and custom-lower ISD::ATOMIC_CMP_SWAP to PPCISD::ATOMIC_CMP_SWAP; that way, it's clear your node has different semantics.


OK, I tend to opt for not introducing target-specific stuff that is opaque to the infrastructure, but if it has to be done, it has to be done. I'll add the PPCISD versions and update the patch asap since I'd certainly like to merge this back into 6.0 as well.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8876
+  // ATOMIC_CMP_SWAP is Legal and ATOMIC_CMP_SWAP_WITH_SUCCESS needs to be
+  // Expanded.
+  return ToExpand ? SDValue() : Op;
----------------
efriedma wrote:
> Do you really need to custom-lower ATOMIC_CMP_SWAP_WITH_SUCCESS, given that we're just going to call into the custom lowering code again with the generated ATOMIC_CMP_SWAP?
Well, unless your patch or Uli's (D41798 or D38215) to zero-extend the operand to the generated `SETCC` lands, I need to do it for both, don't I? I certainly continue to get failures if I don't do this for both nodes without one of those patches.


Repository:
  rL LLVM

https://reviews.llvm.org/D41856