[PATCH] D157389: AutoUpgrade: Use syncscope("agent") atomic.inc/dec intrinsic upgrade

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

In D157389#4569735 <https://reviews.llvm.org/D157389#4569735>, @arsenm wrote:

> In D157389#4569730 <https://reviews.llvm.org/D157389#4569730>, @sameerds wrote:
>
>> What is the "old syncscope parameter" mentioned in the change description? In the actual change and the lit tests, the default "system" scope is being changed to "agent", which superficially looks like an incorrect change.
>
> In the deleted intrinsic. There was an integer syncscope argument which was never actually used. This just needs to try emulating the old intrinsic behavior

Before the change that dropped the intrinsic <https://reviews.llvm.org/rG92ee60b66f581fdd919315da5c6ae631e581b021>, the legalizer looks like it was translating the syncscope parameter from the intrinsic to the emitted atomic. Then the dropping change upgrade the scope to the default system scope. Then in what sense was it not previously used? If the old behaviour produced atomics that only worked in workgroup scope, does that mean that even now, an LLVM program that uses atomic instructions with system scope is not guaranteed to work?

Also, should there be a .bc test demonstrating the auto-upgrade?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157389/new/

https://reviews.llvm.org/D157389