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

[Sean Mollet via Phabricator via llvm-commits]

SeanMollet marked an inline comment as done.
SeanMollet added a comment.

In D152328#4436463 <https://reviews.llvm.org/D152328#4436463>, @phosek wrote:

> I don't think this change is the right approach.

As opposed to the current approach of just emitting them all the time with reckless abandon? This is a sanity check to eliminate targets that are almost certain to fail.

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

I agree that either this or checking with the target class (which as far as I can tell isn't accessible from InstrProfiler) is the ideal solution. But, doing either is well beyond my experience with LLVM and available time.



================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1390
+  // If this assumption is incorrect, additional logic can be added here.
+  return TT.isArch64Bit();
+}
----------------
phosek wrote:
> 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.
Then RV64 without A and ARMv8-A should also be excluded. I don't believe there's a way to get sub-target details from inside this class. Do you know of a way?

Best I can tell, all it has is the triplet.

Note:  the bar is not whether a target supports atomic operations. InstrProfiler.cpp emits 64 bit atomic operations,  which based on the manual, I do not believe Armv8-M supports. 

Ref the Armv8-M architecture reference manual:

https://developer.arm.com/documentation/ddi0553/latest/

Page 252 lists the available Load-Acquire/Store-Release instructions which do not include 64-bit variants.


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

https://reviews.llvm.org/D152328