InstrProf - don't emit 64 bit atomic operations on 32 bit platforms

[SeanMollet]

Summary

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

[SeanMollet]

Phabricator doesn't seem to have the commits from pre GIT days, so here's a link to the original commit that introduced the code in question:

https://github.com/SeanMollet/llvm-project/commit/61ba2e3996120a08deef823dccd7e8d8cd9c4332

[SeanMollet]

The commit that added the lowering to atomics is: https://reviews.llvm.org/D34085

[ellis]

It would be good to test that atomics are not used for targets that don't support them. I think it would look similar to llvm/test/Instrumentation/InstrProfiling/atomic-updates.ll.

Another option would be to throw an error if the user attempts to pass one of the atomic flags (e.g., -instrprof-atomic-counter-update-all, -atomic-first-counter, ...) on targets that don't support atomics. Or maybe we should at least report a warning? I'm not sure what the precedent is.

I'm also adding other reviewers that might have more context.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

454

I think this can be refactored to

if (!targetSupportsAtomic())
  return false;

[SeanMollet]

Updated counter_promo tests to include a target triple that supports the atomic profiler functionality they're testing.
Added an atomic-updates-disabled test with a target triple of a device that doesn't support atomic profiling.
atomic-updates.ll already specified an appropriate target triple that supports atomic profiling.
Made @ellis suggested refactor.

[SeanMollet]

In D152328#4401816, @ellis wrote:

It would be good to test that atomics are not used for targets that don't support them. I think it would look similar to llvm/test/Instrumentation/InstrProfiling/atomic-updates.ll.

Another option would be to throw an error if the user attempts to pass one of the atomic flags (e.g., -instrprof-atomic-counter-update-all, -atomic-first-counter, ...) on targets that don't support atomics. Or maybe we should at least report a warning? I'm not sure what the precedent is.

I'm also adding other reviewers that might have more context.

I added your suggested test and suggested refactor. Good ideas on both, thanks.

As I mentioned, this is my first LLVM contribution, so I'm not familiar with the guiding philosophy. Feel free to advise me if my thought process is off base. I generally prefer the path of least resistance/least change to APIs.

The difficulty with throwing an error is that atomics are now turned on by default. So, it's not the client asking for them in most cases, it's being done internally. Those options aren't exposed by the API and the profiler is used quite extensively in Rust. So, changing the behavior to require passing the flags would require what I suspect is significant rework here and there for no real benefit. Building for a target that supports the atomics, this works exactly as it always did and requires no changes downstream. Building for a target that doesn't support them used to fail, this makes it work. Minimal changes, everything works as expected.

I wouldn't object to a warning, since I don't think that would impair any functionality. I'm happy to add one if I can impose on somebody for a sample of the right way to throw one.

[davidxl]

For target without hw instruction for fetch add, intrinsic like __sync_fetch_and_add() can be used instead -- it will be slow but workable. Why disabling it?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

456

why disabling counter promotion? It is a different from atomic counter update.

[SeanMollet]

In D152328#4435713, @davidxl wrote:

For target without hw instruction for fetch add, intrinsic like __sync_fetch_and_add() can be used instead -- it will be slow but workable. Why disabling it?

Because the whole "lowering to an atomic operation" thing exists for the purpose of improving performance, A slow replacement doesn't accomplish that, so why add more code and potential bugs when there's no benefit?

[davidxl]

Regarding improving performance, it is more about improving the optimized build (profile-use) performance instead of the performance of the instrumented build. This is because the atomic update allows more precise counter update in multi-threaded programs.

[SeanMollet]

In D152328#4435805, @davidxl wrote:

Regarding improving performance, it is more about improving the optimized build (profile-use) performance instead of the performance of the instrumented build. This is because the atomic update allows more precise counter update in multi-threaded programs.

I agree that it's about improving the profiler performance. The full software approach would improve precision while profling multi-threaded programs at the cost of substantially slowing down the resulting program and potentially impacting the timing.

In general, the targets this change would apply to are older or embedded, slow and usually single core systems. In my particular case, I'm working on a 600 Mhz embedded Mips system. It's already slow and the code isn't very threaded. Given the choice, I'm perfectly happy to give up some accuracy for faster profiling.

If I was building something for a server or modern laptop/desktop/mobile phone, I'd make the opposite choice. But, they all support hardware atomics, so it's moot.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

456

Because it also sometimes lowers to an atomic counter update. As an alternative, it could be re-worked to not do that, but then it doesn't provide meaningful benefit.

[davidxl]

It might be better to handle this at the start of InstrProfiling::run (to override the Atomic related options with a proper warning).

Another thing is that what if an user wants to enable the emulated atomic update even when it is slow? You may want to introduce another internal option to control that : e.g. useAtomicEmulation (default to false). If it is true, do not override the option even when the target does not have hardware impl.

[SeanMollet]

In D152328#4436006, @davidxl wrote:

It might be better to handle this at the start of InstrProfiling::run (to override the Atomic related options with a proper warning).

I agree that a warning is a good idea. Can you point me to a good sample of the right/correct/prefered way to word, format and output one?

Another thing is that what if an user wants to enable the emulated atomic update even when it is slow? You may want to introduce another internal option to control that : e.g. useAtomicEmulation (default to false). If it is true, do not override the option even when the target does not have hardware impl.

That's a reasonable approach. Is it actually necessary now? Considering that currently profiling isn't available at all on the targets in question, and I'm apparently the first person to notice that in the 9 years since the original change; I don't think the group of people that care is very large. If somebody asks for it in the future, I'll write it.

[davidxl]

There is an example in InstrProfiling.cpp about missing debug information warning.

For the emulation feature, it is ok to delay it -- but please do a TODO as a comment.

[SeanMollet]

In D152328#4436143, @davidxl wrote:

There is an example in InstrProfiling.cpp about missing debug information warning.

For the emulation feature, it is ok to delay it -- but please do a TODO as a comment.

Perfect, thanks.

One other question, I want to set a flag so as to not spam the user with warnings. Is there a standard/expected way to do that in LLVM?

[ellis]

In D152328#4436143, @davidxl wrote:

There is an example in InstrProfiling.cpp about missing debug information warning.

For the emulation feature, it is ok to delay it -- but please do a TODO as a comment.

Yeah I kind of regret adding this warning as it is. It is definitely pretty spammy and the only way to suppress it is to suppress all backend warnings.

[SeanMollet]

In D152328#4436231, @ellis wrote:

In D152328#4436143, @davidxl wrote:

There is an example in InstrProfiling.cpp about missing debug information warning.

For the emulation feature, it is ok to delay it -- but please do a TODO as a comment.

Yeah I kind of regret adding this warning as it is. It is definitely pretty spammy and the only way to suppress it is to suppress all backend warnings.

I was thinking a static flag that gets set on the first warning and then skip subsequent ones. I could wrap that other warning as well. Just didn't want to go randomly adding static variables since I have no idea about the policies surrounding things like that.

[ellis]

In D152328#4436240, @SeanMollet wrote:

In D152328#4436231, @ellis wrote:

In D152328#4436143, @davidxl wrote:

There is an example in InstrProfiling.cpp about missing debug information warning.

For the emulation feature, it is ok to delay it -- but please do a TODO as a comment.

Yeah I kind of regret adding this warning as it is. It is definitely pretty spammy and the only way to suppress it is to suppress all backend warnings.

I was thinking a static flag that gets set on the first warning and then skip subsequent ones. I could wrap that other warning as well. Just didn't want to go randomly adding static variables since I have no idea about the policies surrounding things like that.

Even with a static flag, that doesn't stop the warning from showing up from each clang invocation if you aren't using LTO.

By the way, why not simply use -fprofile-update=single (which I think will set Options.Atomic = false) for your use case? Now that I look closely, all the atomic flags seem to be false by default. Where are you seeing atomic instructions emitted for you?

[davidxl]

My understanding is that atomic update is enabled in some shared build environment thus causing problems for the 32 bit target. If that is not the case, it is perhaps better to use -fprofile-update option to control the behavior as suggested by Ellis.

[SeanMollet]

In D152328#4436337, @ellis wrote:

Even with a static flag, that doesn't stop the warning from showing up from each clang invocation if you aren't using LTO.

True. At least it would only be once per file. Without a flag, it would throw at least twice per file. I don't know the call pattern into PGO, so maybe more than that. At least twice though, which seems worth a flag. Open to suggestions, I only want to do the "right" thing..

By the way, why not simply use -fprofile-update=single (which I think will set Options.Atomic = false) for your use case? Now that I look closely, all the atomic flags seem to be false by default. Where are you seeing atomic instructions emitted for you?

Forgive me if I get the exact wording of this wrong, this is the my first foray into LLVM.

Because the intrinsics aren't being emitted by the backend. They're being converted to an atomic directly by InstrProfiling.cpp which doesn't look at that option.

PassBuilderPipelines.cpp line 787 effectively sets the default for InstrProfiling.cpp to true. DoCounterPromotion=true uses atomics.

// Add the profile lowering pass.
InstrProfOptions Options;
if (!ProfileFile.empty())
  Options.InstrProfileOutput = ProfileFile;
// Do counter promotion at Level greater than O0.
Options.DoCounterPromotion = true;
Options.UseBFIInPromotion = IsCS;
MPM.addPass(InstrProfiling(Options, IsCS));

[SeanMollet]

In D152328#4436381, @davidxl wrote:

My understanding is that atomic update is enabled in some shared build environment thus causing problems for the 32 bit target. If that is not the case, it is perhaps better to use -fprofile-update option to control the behavior as suggested by Ellis.

It's enabled at any optimization level greater than O0 (I pasted the offending code in the previous reply). As far as I can tell, profile-update has no effect on it.

[ellis]

In D152328#4436415, @SeanMollet wrote:

In D152328#4436337, @ellis wrote:

Even with a static flag, that doesn't stop the warning from showing up from each clang invocation if you aren't using LTO.

True. At least it would only be once per file. Without a flag, it would throw at least twice per file. I don't know the call pattern into PGO, so maybe more than that. At least twice though, which seems worth a flag. Open to suggestions, I only want to do the "right" thing..

By the way, why not simply use -fprofile-update=single (which I think will set Options.Atomic = false) for your use case? Now that I look closely, all the atomic flags seem to be false by default. Where are you seeing atomic instructions emitted for you?

Forgive me if I get the exact wording of this wrong, this is the my first foray into LLVM.

Because the intrinsics aren't being emitted by the backend. They're being converted to an atomic directly by InstrProfiling.cpp which doesn't look at that option.

PassBuilderPipelines.cpp line 787 effectively sets the default for InstrProfiling.cpp to true. DoCounterPromotion=true uses atomics.

// Add the profile lowering pass.
InstrProfOptions Options;
if (!ProfileFile.empty())
  Options.InstrProfileOutput = ProfileFile;
// Do counter promotion at Level greater than O0.
Options.DoCounterPromotion = true;
Options.UseBFIInPromotion = IsCS;
MPM.addPass(InstrProfiling(Options, IsCS));

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.

[SeanMollet]

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

[phosek]

In D152328#4436433, @SeanMollet wrote:

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

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

1390

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.

[SeanMollet]

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

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

1390

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.

[davidxl]

In D152328#4436433, @SeanMollet wrote:

In 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);

This is guarded by option atomic-counter-update-promoted option which is false by default. Can you debug your build see how it is set to true?

I added the check around line 743 as well for sanity reasons.

[SeanMollet]

OK, I got brought back to this by a comment on my downstream rust issue.

@Zalathar kindly pointed out the commit on the rust side that is turning this on (blindly).

Turn on:
https://github.com/rust-lang/rust/pull/111469/files

compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp

    if (InstrProfileOutput) {
      Options.InstrProfileOutput = InstrProfileOutput;
    }
    // cargo run tests in multhreading mode by default
    // so use atomics for coverage counters
    Options.Atomic = true;
    MPM.addPass(InstrProfiling(Options, false));
  }
);

I'm going to investigate down there and see if there's a way for it to be smarter about turning this on. With that said though, I'm still of the mind that a sanity check in LLVM for obvious failures isn't a bad idea. Thoughts?

llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h

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

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

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

   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 {

 
   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();
+}

llvm/test/Instrumentation/InstrProfiling/atomic-updates-disabled.ll

+; RUN: opt < %s -S -passes=instrprof -instrprof-atomic-counter-update-all | FileCheck %s
+; This test purposely uses a target triple that doesn't support the atomic update used by InstrProf
+; It shouldn't end up with an atomic update instruction
+
+target triple = "mips-unknown-linux-gnu"
+
+@__profn_foo = private constant [3 x i8] c"foo"
+
+; CHECK-LABEL: define void @foo
+; CHECK-NOT: atomicrmw add ptr @__profc_foo, i64 1 monotonic
+define void @foo() {
+  call void @llvm.instrprof.increment(ptr @__profn_foo, i64 0, i32 1, i32 0)
+  ret void
+}
+
+declare void @llvm.instrprof.increment(ptr, i64, i32, i32)

llvm/test/Instrumentation/InstrProfiling/atomic-updates.ll

 ; RUN: opt < %s -S -passes=instrprof -instrprof-atomic-counter-update-all | FileCheck %s
+; This test purposely uses a target triple that does support the atomic update used by InstrProf
 
 target triple = "x86_64-apple-macosx10.10.0"
 

llvm/test/Transforms/PGOProfile/counter_promo.ll

 ; RUN: opt < %s --passes=pgo-instr-gen,instrprof -do-counter-promotion=true -skip-ret-exit-block=0 -S | FileCheck --check-prefix=PROMO --check-prefix=NONATOMIC_PROMO %s
 ; RUN: opt < %s --passes=pgo-instr-gen,instrprof -do-counter-promotion=true -atomic-counter-update-promoted -skip-ret-exit-block=0 -S | FileCheck --check-prefix=PROMO --check-prefix=ATOMIC_PROMO %s
 
+target triple = "x86_64-apple-macosx10.10.0"
+
 define void @foo(i32 %n, i32 %N) {
 ; PROMO-LABEL: @foo
 ; PROMO: {{.*}} = load {{.*}} @__profc_foo{{.*}} 3)

llvm/test/Transforms/PGOProfile/counter_promo_exit_merge.ll

 ; RUN: opt < %s --passes=instrprof -do-counter-promotion=true -speculative-counter-promotion-max-exiting=3 -S | FileCheck --check-prefix=PROMO %s
 
+target triple = "x86_64-apple-macosx10.10.0"
+
 $__llvm_profile_raw_version = comdat any
 
 @g = common local_unnamed_addr global i32 0, align 4

llvm/test/Transforms/PGOProfile/counter_promo_mexits.ll

 ; RUN: opt < %s --passes=pgo-instr-gen,instrprof -do-counter-promotion=true -speculative-counter-promotion-max-exiting=3 -S | FileCheck --check-prefix=PROMO %s
 
+target triple = "x86_64-apple-macosx10.10.0"
+
 @g = common local_unnamed_addr global i32 0, align 4
 
 define void @foo(i32 %arg) local_unnamed_addr {

llvm/test/Transforms/PGOProfile/counter_promo_nest-inseltpoison.ll

 ; TEST that counter updates are promoted outside the whole loop nest
 ; RUN: opt < %s --passes=pgo-instr-gen,instrprof -do-counter-promotion=true -S | FileCheck --check-prefix=PROMO  %s 
 
+target triple = "x86_64-apple-macosx10.10.0"
+
 @g = common local_unnamed_addr global i32 0, align 4
 @c = local_unnamed_addr global i32 10, align 4
 

llvm/test/Transforms/PGOProfile/counter_promo_nest.ll

 ; TEST that counter updates are promoted outside the whole loop nest
 ; RUN: opt < %s --passes=pgo-instr-gen,instrprof -do-counter-promotion=true -S | FileCheck --check-prefix=PROMO  %s 
 
+target triple = "x86_64-apple-macosx10.10.0"
+
 @g = common local_unnamed_addr global i32 0, align 4
 @c = local_unnamed_addr global i32 10, align 4
 

llvm/test/Transforms/PGOProfile/not_promote_ret_exit.ll

 ; RUN: opt < %s -passes=instrprof -S -do-counter-promotion=1 -skip-ret-exit-block=1 | FileCheck %s --check-prefixes=CHECK,SKIP
 ; RUN: opt < %s -passes=instrprof -S -do-counter-promotion=1 -skip-ret-exit-block=0 | FileCheck %s --check-prefixes=CHECK,NOTSKIP
 
+target triple = "x86_64-apple-macosx10.10.0"
+
 $__llvm_profile_raw_version = comdat any
 
 @bar = dso_local local_unnamed_addr global i32 0, align 4