[PATCH] D41798: [LegalizeDAG] Fix ATOMIC_CMP_SWAP_WITH_SUCCESS legalization.

Nemanja Ivanovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 8 08:33:26 PST 2018


nemanjai added a comment.

In https://reviews.llvm.org/D41798#969786, @uweigand wrote:

> This patch seems to be identical to the one I proposed here:
>  https://reviews.llvm.org/D38215
>
> (While as mentioned there, this is no longer needed on SystemZ, I still think this is a correct change.)
>
> As to Nemanja's comments, in the default expansion of ATOMIC_CMP_SWAP_WITH_SUCCESS, the result of an ATOMIC_CMP_SWAP call is compared with the compare value.  Since the result of ATOMIC_CMP_SWAP may be differently extended depending on the platform (this is what TLI.getExtendForAtomicOps is all about), the compare value must be extended in the same way.  E.g. on Mips, where TLI.getExtendForAtomicOps is SIGN_EXTEND, the compare value must likewise be always sign-extended, or else the comparison will fail.
>
> Given that which extension is correct depends on the platform, it doesn't seem to make much sense to me to do an unconditional zero-extension of the compare value in common code ahead of time.


Fair enough. I agree.

> In any case, I'm not sure why the *input* to ATOMIC_CMP_SWAP should be extended in any way in the first place: the back-end gets told the actual type explicitly, and if any extension is necessary due to the particular way a back-end implements the operation, the back-end can just do that extension itself.

I'm not sure what you mean here. The `i8` and `i16` types are not legal on PPC. As such, type legalization will undoubtedly expand operands of those types. If you look at the debug dumps I posted above, you'll notice that pre-type-legalization, the inputs are `i16` and post-type-legalization, they're `i32`.
I think the gist of the issue here is that type legalization should do the right thing - operation legalization is probably later than the optimal place for this. Although, I agree that the operands should probably be extended according to `TLI.getExtendForAtomicOps()`.


Repository:
  rL LLVM

https://reviews.llvm.org/D41798





More information about the llvm-commits mailing list