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

Wed Jun 7 06:30:13 PDT 2023

SeanMollet added a comment.

In D152328#4401816 <https://reviews.llvm.org/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.

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

https://reviews.llvm.org/D152328