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

Wed Aug 9 15:58:55 PDT 2023

arsenm added a comment.

In D157389#4571828 <https://reviews.llvm.org/D157389#4571828>, @sameerds wrote:

> 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.

It wasn't. The MachineMemOperand properties come from getTgtMemIntrinsic and never read these. Switching the implementation fixed bugs from assuming workgroup scope, there weren't enough waits.

> 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?

Switching to atomicrmw fixed all the non-system scope cases. This fixes it so you still get the instruction through the intrinsic after D157437 <https://reviews.llvm.org/D157437> (although unfortunately you lose that if you upgraded with a previous version), but system scope would continue to not work.

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

This is the "bc" test. All the intrinsic autoupgrade tests work by assembling and then disassembling text bitcode, there's no encoding change being tested

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

https://reviews.llvm.org/D157389