Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support more AtomicRWOps #138

Merged
merged 4 commits into from
Jul 15, 2024
Merged

Conversation

tomsmeding
Copy link
Contributor

The current list of operations for atomicrmw (AtomicRWOp) was up to date for LLVM 8, but since then more operations have been added. By now (LLVM 19), 6 more have arrived (documentation). This PR adds them, and predicates their pretty-printing on the minimum LLVM version that has them.

Caveats:

  1. Because data constructors are declared at compile time and we only know the target LLVM version at runtime, all we can do is error if an operation is used that the target LLVM version does not support. This is unfortunate, but I don't see a better option here.
  2. I deduced minimum LLVM versions simply by looking at the LLVM LangRef documentation for atomicrmw for a whole bunch of LLVM versions. I did not actually test that the documentation corresponds with reality.

(Context: as an experiment, I'm trying to convert the Accelerate LLVM backend to use llvm-pretty instead of the llvm-hs bindings with the aim of reducing maintenance overhead for the Accelerate maintainers. No guarantees that we'll actually be merging this migration.)

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

1. Because data constructors are declared at compile time and we only know the target LLVM version at runtime, all we can do is `error` if an operation is used that the target LLVM version does not support. This is unfortunate, but I don't see a better option here.

That sounds quite reasonable to me, actually. In fact, we really ought to be doing this more often in the Text.LLVM.PP code than we current are. Of course, it would be a big task to identify all of the places where we're emitting LLVM IR that won't work on certain old LLVM versions, but I like the idea of using onlyOnLLVM to identify these places going forward.

2. I deduced minimum LLVM versions simply by looking at the LLVM LangRef documentation for `atomicrmw` for a whole bunch of LLVM versions. I did not actually test that the documentation corresponds with reality.

That is exactly how I do things, so you're in good company :)

(Context: as an experiment, I'm trying to convert the Accelerate LLVM backend to use llvm-pretty instead of the llvm-hs bindings with the aim of reducing maintenance overhead for the Accelerate maintainers. No guarantees that we'll actually be merging this migration.)

That sounds like a cool project! And it's nice to hear that other people are getting use of llvm-pretty. Please don't hesitate to file issues if there are other things that accelerate-llvm needs that llvm-pretty doesn't currently provide.

src/Text/LLVM/AST.hs Outdated Show resolved Hide resolved
@tomsmeding
Copy link
Contributor Author

tomsmeding commented Jul 15, 2024

That sounds quite reasonable to me, actually. In fact, we really ought to be doing this more often in the Text.LLVM.PP code than we current are. Of course, it would be a big task to identify all of the places where we're emitting LLVM IR that won't work on certain old LLVM versions, but I like the idea of using onlyOnLLVM to identify these places going forward.

That makes sense. Given that Accelerate's users will typically not be using very old versions of LLVM, I don't think we'll be running into undocumented support limits, so from our side this will probably mostly stay as-is. But let's try to do it right for new constructors, at least. :)

That sounds like a cool project! And it's nice to hear that other people are getting use of llvm-pretty. Please don't hesitate to file issues if there are other things that accelerate-llvm needs that llvm-pretty doesn't currently provide.

Thanks so much! Whether we need much more depends on whether we're also going to convert the CUDA backend (I'm not even sure whether that's feasible in the first place -- I'm not a CUDA API expert). If so, from memory, we may want these things at some point: half floats, some additional function attributes, and volatile loads and stores, as well as inline assembly. The current code also adds function attributes to call instructions (which llvm-pretty does not support), but I'm not sure whether those are actually needed -- contrary to what LLVM's documentation suggests, one can put function attributes on declare statements, and that ought to be enough, in theory.

I propose moving this disclaimer to the module Haddocks and generalizing it a bit. We don't rule out all possible invalid combinations just yet when pretty-printing, but we could at least encourage users to check that what they are attempting to pretty-print is in fact valid on their chosen version of LLVM.

I did so; I'm not sure about the wording. Feel free to edit or suggest changes.

I also reworded the support bounds comments to use the phrasing "Introduced in", to be consistent with the other places in the module where similar support bounds are indicated. I kind of like the plain "LLVM >= 9" phrasing more, because my brain reads that as "oops I should check whether that's old enough for me" whereas it reads "Introduced in ..." as a skippable historical remark. But that's my problem, not anyone else's. :)

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor Haddock suggestions, but otherwise this looks good to me.

src/Text/LLVM/AST.hs Outdated Show resolved Hide resolved
src/Text/LLVM/PP.hs Outdated Show resolved Hide resolved
src/Text/LLVM/PP.hs Outdated Show resolved Hide resolved
@tomsmeding
Copy link
Contributor Author

Sounds good, applied. :)

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@RyanGlScott RyanGlScott merged commit a815d8c into GaloisInc:master Jul 15, 2024
1 check passed
@tomsmeding tomsmeding deleted the more-atomicrwop branch July 15, 2024 20:56
RyanGlScott added a commit to GaloisInc/llvm-pretty-bc-parser that referenced this pull request Jul 25, 2024
This adapts `llvm-pretty-bc-parser` to new atomic operations introduced in LLVM
9 and later:

* This bumps the `llvm-pretty` submodule to incorporate the changes from
  GaloisInc/llvm-pretty#138 and
  GaloisInc/llvm-pretty#140.
* This updates the parsing code in `Data.LLVM.BitCode.IR.Function` to account
  for atomic `fadd`, `fsub`, `fmax`, `fmin`, `uinc_wrap`, and `udec_wrap`
  operations.
* This ensures that all atomic operations have a corresponding `disasm-test`
  test case.
RyanGlScott added a commit to GaloisInc/llvm-pretty-bc-parser that referenced this pull request Jul 25, 2024
This adapts `llvm-pretty-bc-parser` to new atomic operations introduced in LLVM
9 and later:

* This bumps the `llvm-pretty` submodule to incorporate the changes from
  GaloisInc/llvm-pretty#138 and
  GaloisInc/llvm-pretty#140.
* This updates the parsing code in `Data.LLVM.BitCode.IR.Function` to account
  for atomic `fadd`, `fsub`, `fmax`, `fmin`, `uinc_wrap`, and `udec_wrap`
  operations.
* This ensures that all atomic operations have a corresponding `disasm-test`
  test case.
RyanGlScott added a commit to GaloisInc/llvm-pretty-bc-parser that referenced this pull request Jul 25, 2024
This adapts `llvm-pretty-bc-parser` to new atomic operations introduced in LLVM
9 and later:

* This bumps the `llvm-pretty` submodule to incorporate the changes from
  GaloisInc/llvm-pretty#138 and
  GaloisInc/llvm-pretty#140.
* This updates the parsing code in `Data.LLVM.BitCode.IR.Function` to account
  for atomic `fadd`, `fsub`, `fmax`, `fmin`, `uinc_wrap`, and `udec_wrap`
  operations.
* This ensures that all atomic operations have a corresponding `disasm-test`
  test case.
RyanGlScott added a commit to GaloisInc/llvm-pretty-bc-parser that referenced this pull request Jul 25, 2024
This adapts `llvm-pretty-bc-parser` to new atomic operations introduced in LLVM
9 and later:

* This bumps the `llvm-pretty` submodule to incorporate the changes from
  GaloisInc/llvm-pretty#138 and
  GaloisInc/llvm-pretty#140.
* This updates the parsing code in `Data.LLVM.BitCode.IR.Function` to account
  for atomic `fadd`, `fsub`, `fmax`, `fmin`, `uinc_wrap`, and `udec_wrap`
  operations.
* This ensures that all atomic operations have a corresponding `disasm-test`
  test case.
RyanGlScott added a commit to GaloisInc/llvm-pretty-bc-parser that referenced this pull request Jul 25, 2024
This adapts `llvm-pretty-bc-parser` to new atomic operations introduced in LLVM
9 and later:

* This bumps the `llvm-pretty` submodule to incorporate the changes from
  GaloisInc/llvm-pretty#138 and
  GaloisInc/llvm-pretty#140.
* This updates the parsing code in `Data.LLVM.BitCode.IR.Function` to account
  for atomic `fadd`, `fsub`, `fmax`, `fmin`, `uinc_wrap`, and `udec_wrap`
  operations.
* This ensures that all atomic operations have a corresponding `disasm-test`
  test case.
RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Jul 25, 2024
This adapts `crucible-llvm` to new atomic operations introduced in LLVM 9 and later:

* This bumps the `llvm-pretty` submodule to incorporate the changes from
  GaloisInc/llvm-pretty#138 and
  GaloisInc/llvm-pretty#140.
* This bumps the `llvm-pretty-bc-parser` submodule to incorporate changes from
  GaloisInc/llvm-pretty-bc-parser#274.
* This updates `crucible-llvm`'s semantics for the `atomicrmw` instruction to
  account for atomic `fadd`, `fsub`, `fmax`, `fmin`, `uinc_wrap`, and `udec_wrap` operations.
* This ensures that all atomic operations have a corresponding `crux-llvm` test case.
RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Jul 26, 2024
This adapts `crucible-llvm` to new atomic operations introduced in LLVM 9 and later:

* This bumps the `llvm-pretty` submodule to incorporate the changes from
  GaloisInc/llvm-pretty#138 and
  GaloisInc/llvm-pretty#140.
* This bumps the `llvm-pretty-bc-parser` submodule to incorporate changes from
  GaloisInc/llvm-pretty-bc-parser#274.
* This updates `crucible-llvm`'s semantics for the `atomicrmw` instruction to
  account for atomic `fadd`, `fsub`, `fmax`, `fmin`, `uinc_wrap`, and `udec_wrap` operations.
* This ensures that all atomic operations have a corresponding `crux-llvm` test case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants