[PATCH] D152328: InstrProf - don't emit 64 bit atomic operations on 32 bit platforms

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

In D152328#4436433 <https://reviews.llvm.org/D152328#4436433>, @SeanMollet wrote:

> In D152328#4436425 <https://reviews.llvm.org/D152328#4436425>, @ellis wrote:
>
>> I still don't see how `DoCounterPromotion=true` emits atomics. As far as I can tell, all atomic instructions emitted in `InstrProfiling.cpp` are guarded behind an atomic flag that defaults to false.
>
> I honestly couldn't figure it out either. If I change that to =false or comment that line out, they don't get emitted. Somehow AtomicCounterUpdatePromoted is getting set to true.
>
> The offending emitter is InstrProfiling.cpp line 198:
>
>   Builder.CreateAtomicRMW(AtomicRMWInst::Add, Addr, LiveInValue,
>                           MaybeAlign(),
>                           AtomicOrdering::SequentiallyConsistent);
>
> I added the check around line 743 as well for sanity reasons.

I think we should first figure out why that's the case. In any case, I don't think this change is the right approach. If we wanted to use atomics depending on whether the target supports them or not, we'll need to postpone the lowering until later in the backend.



================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1390
+  // If this assumption is incorrect, additional logic can be added here.
+  return TT.isArch64Bit();
+}
----------------
I don't think this is correct, there are many 64-bit targets that don't have atomics, for example RV64 without the A extension, or even ARMv8-A. Conversely, there are 32-bit targets that support atomics such as Armv8-M.


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

https://reviews.llvm.org/D152328