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

[Sean Mollet via Phabricator via llvm-commits]

SeanMollet created this revision.
SeanMollet added a reviewer: asbirlea.
SeanMollet added a project: LLVM.
Herald added subscribers: Enna1, ellis, pengfei, atanasyan, hiraditya, kristof.beyls, arichardson, sdardis.
Herald added a project: All.
SeanMollet requested review of this revision.
Herald added a subscriber: llvm-commits.

Please forgive any mistakes in form, this is my first contribution to LLVM.

Starting with this commit: 61ba2e3996120a08deef823dccd7e8d8cd9c4332
Profiling was implemented with an intrinsic and lowering for it was consolidated in InstrProfiling.cpp

Optional lowering of the counter increment via (faster) atomic operations was also added at this time. Per the notes from the original commit, the primary goal here was consolidating the manual lowering of the profiling code into one place so it could be hand optimized.

As best I can tell, later on, c3ddc13d7d631a29b55f82c7cc7a9008bf89b1f4 <https://reviews.llvm.org/rGc3ddc13d7d631a29b55f82c7cc7a9008bf89b1f4> then sets DoCounterPromotion = true unless O0.

This breaks compilation with the profiler turned on for platforms that don't support 64 bit atomic operations.

Since the atomic operations are an optimization anyway and are only useful if the hardware has support, my patch checks for that support (in an admittedly naive way, but it should be sufficient) and disables the atomic optimizations if they're not going to work.

The resulting output of the compiler (including the profiling) works correctly on my target mipsel platform. It also produces correct output on x86_64, aarch64 and arm.

The existing tests for the profiler will adequately cover my changes as well. Prior to my change, both the atomic and non atomic variations of the code were used even within the same job at various places.

Alina, you approved several commits for this section of code, so I'm assuming you know it well. I'm open to suggestions if you have a better approach. Thanks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152328

Files:
  llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===================================================================
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -451,10 +451,13 @@
 }

 bool InstrProfiling::isCounterPromotionEnabled() const {
-  if (DoCounterPromotion.getNumOccurrences() > 0)
-    return DoCounterPromotion;
+  if (targetSupportsAtomic()) {
+    if (DoCounterPromotion.getNumOccurrences() > 0)
+      return DoCounterPromotion;

-  return Options.DoCounterPromotion;
+    return Options.DoCounterPromotion;
+  }
+  return false;
 }

 void InstrProfiling::promoteCounterLoadStores(Function *F) {
@@ -714,8 +717,9 @@
   auto *Addr = getCounterAddress(Inc);

   IRBuilder<> Builder(Inc);
-  if (Options.Atomic || AtomicCounterUpdateAll ||
-      (Inc->getIndex()->isZeroValue() && AtomicFirstCounter)) {
+  if (targetSupportsAtomic() &&
+      (Options.Atomic || AtomicCounterUpdateAll ||
+       (Inc->getIndex()->isZeroValue() && AtomicFirstCounter))) {
     Builder.CreateAtomicRMW(AtomicRMWInst::Add, Addr, Inc->getStep(),
                             MaybeAlign(), AtomicOrdering::Monotonic);
   } else {
@@ -1288,3 +1292,12 @@

   appendToGlobalCtors(*M, F, 0);
 }
+
+bool InstrProfiling::targetSupportsAtomic() const {
+  // As far as I can tell, we don't have access to specific enough target
+  // details here to really know if they support the atomic add or not. It seems
+  // like a safe assumption though that only 64 bit targets will support the 64
+  // bit atomic operation we're going to inject.
+  // If this assumption is incorrect, additional logic can be added here.
+  return TT.isArch64Bit();
+}
Index: llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h
===================================================================
--- llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h
+++ llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h
@@ -134,6 +134,10 @@
   /// Create a static initializer for our data, on platforms that need it,
   /// and for any profile output file that was specified.
   void emitInitialization();
+
+  /// Check if our target supports 8 byte/ 64 bit Atomics. Don't use
+  /// them if it doesn't.
+  bool targetSupportsAtomic() const;
 };

 } // end namespace llvm


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D152328.529112.patch
Type: text/x-patch
Size: 2346 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230607/fa69e0d5/attachment.bin>