Page MenuHomePhabricator

[MLIR] LLVM dialect: Add llvm.atomicrmw
ClosedPublic

Authored by flaub on Jan 14 2020, 3:49 PM.

Details

Summary

This op is the counterpart to LLVM's atomicrmw instruction. Note that
volatile and syncscope attributes are not yet supported.

This will be useful for upcoming parallel versions of affine.for and generally
for reduction-like semantics.

Diff Detail

Event Timeline

flaub created this revision.Jan 14 2020, 3:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 3:49 PM
jfb added inline comments.Jan 14 2020, 4:15 PM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751

Could you call this one relaxed, so it matches C++, instead of the LLVM IR name?

jfb added a subscriber: __simt__.Jan 14 2020, 4:15 PM

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

flaub marked an inline comment as done.Jan 14 2020, 4:29 PM
flaub added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751

I don't have a strong intuition on this, but it seems like it should match LLVM IR faithfully since the LLVM dialect in MLIR should basically mirror it. Also, the context for this change is to support other MLIR dialect lowerings (such as the Affine dialect), and thus don't have any relationship to C++ (except perhaps distantly).

I'd suggest that we call it relaxed in a C++ dialect (or some other frontend), which would lower to this one.

jfb added a subscriber: jyasskin.Jan 14 2020, 4:35 PM
jfb added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751

acquire, release, ace_rel, and seq_cst came from C++ (and then C). @jyasskin might remember why monotonic wasn't called relaxed... but IMO it would be best to match the C++ naming for all these orderings, not all but one.

flaub marked an inline comment as done.Jan 14 2020, 4:37 PM
flaub added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751

I see, I took the namings from: https://llvm.org/docs/LangRef.html#ordering

But if that's out of date, I can update it.

Ideally, this whole dialect would be mechanically generated from LLVM IR sources/td, so whichever name matches the existing IR is probably what we should use.

flaub marked an inline comment as done.Jan 14 2020, 4:40 PM
flaub added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751
ftynse requested changes to this revision.Jan 15 2020, 1:22 AM

Thanks! Most of my comments are minor.

Please also add a translation test to test/Target/llvmir.mlir.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
729

This is unfortunate that this is conflicting with C++ keywords. I differentiated between enum case names (capitalized and thus non-conflicting) and the printed syntax for linkage attributes. That required boilerplate switches in parsing and printing though. I'm fine with these names, but we should consider a general solution for this problem eventually.

751

I am quite strongly opposed to diverging from LLVM names here since the LLVM dialect is modeling LLVM IR, not C++. If we change the name in LLVM IR, we can do the same in the dialect. I have a vague preference for reusing enum names rather than IR keywords.

The documentation in LLVM seems in sync with the code.

Ideally, this whole dialect would be mechanically generated from LLVM IR sources/td, so whichever name matches the existing IR is probably what we should use.

Yes, except that LLVM does not have a .td for instructions, only intrinsics. I am (lazily) thinking in the direction of having a single source of truth for instructions.

766

Nit: the name op is unfortunate as it is often used as short for the current operation.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1377

We tend to use single quotes for single characters (there's an overload on operator<< in some cases).

1379

Dereferencing should not be necessary anymore since mlir::Value is now a value-type.

1433

You can declare op and ordering as StringAttr directly, OpAsmParser does the check for you then.

1436

It would be unclear to the user what 'ordering' means because this word does not appear in the syntax. This code should disappear anyway if you use the typed parser function.

1448

Nit: just write here directly instead of declaring a new vector of attributes?

1454

Nit: return op == AtomicBinOp::fadd || op == AtomicBinOp::fsub; looks 7 times shorter

1464

Auto-generated verifier runs before this and it already checked that all types are LLVMType. Just use cast here and drop the null checks below.

1466

I'd prefer identifying operands using their position. It may sound counter-intuitive, but it is never clear from the IR itself or the error message which operand relates to each name. And counting is easier and less context-switching then opening the doc to check the names, and then still counting in the IR to fix the right operand.

1475–1479

Please add the check that arguments are power-of-two integers otherwise (https://llvm.org/docs/LangRef.html#i-atomicrmw), except for xchg that accepts both floats and integers.

mlir/test/Dialect/LLVMIR/roundtrip.mlir
224–225

I'd rather just pass the pointer in the function to make sure we only check the operation we care about here (atomicrmw)

226

The {volatile} part does not seem necessary for the test.

226

Please avoid using SSA names in the CHECK strings, there are no guarantees they are preserved. Use %{{.*}} instead or capture the names with %[[name-in-tablegen:.*]], see https://mlir.llvm.org/getting_started/TestingGuide/ for more detail.

This revision now requires changes to proceed.Jan 15 2020, 1:22 AM
flaub marked 8 inline comments as done.Jan 15 2020, 2:08 AM

Thanks for the review :)

I'll try to follow up tomorrow and also look at writing a translation test (I wasn't aware of it, thanks for the heads up).

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
766

Is bin_op reasonable?

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1379

Oops, good catch.

1433

Good to know, I'll do that in my next round.

I was attempting to follow other code in this file that looked like it was doing a similar thing.

1448

I don't understand what this comment means.

Actually I was attempting to follow the other custom parser impls in this file.

1454

Except it makes it much harder to maintain. This style, while longer, allows one to easily add a new case when a change in the IR arrives, and makes diffs much easier to comprehend too.

On a minor note, this style matches the exact same function in the LLVM IR:
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/Instructions.h#L775

1464

Fair enough

mlir/test/Dialect/LLVMIR/roundtrip.mlir
224–225

Good idea

226

This was leftover from an earlier attempt to implement a volatile attribute to match the LLVM IR. However, this seems impossible in the llvm builder's present state. There's no way it seems to avoid C++ keyword issues. If I had access to a special $_op expansion then I could write a builder that calls on the op directly to access the attribute.

For now I'll drop the volatile in this test and we can figure out a plan on how to support C++ keyword conflicts more generally.

ftynse added inline comments.Jan 15 2020, 2:24 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
766

Works for me

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1433

This code is older than that function. You are very welcome to modernize the rest in a separate diff :)

1448

parser.parseAttribute(ordering, "ordering", result.attributes)and so on, using result.attributes instead of attrs.

1454

I don't expect much change happening to this part any time soon, but okay to keep as is.

jyasskin added inline comments.Jan 15 2020, 10:11 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751

Since LLVM needed to support Java as well as C++, I felt that relaxed didn't do a good enough job of expressing the difference between their lowest levels of atomicity. I drew "monotonic" from the 4th portability assumption in the "correctness argument" in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2325.html (though that paper doesn't actually suggest using the term as a name for anything). Java non-volatile variables provide defined behavior for racy access, but they don't provide this property, so they got the unordered term.

flaub marked 23 inline comments as done.Jan 15 2020, 4:30 PM
flaub added inline comments.
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1448

The issue is that we need to convert from a string representation to an integer one.

However, I think I've cleaned it up by parsing into a StringAttr and then doing the conversion followed by:

result.addAttribute("bin_op", binOpAttr);
flaub updated this revision to Diff 238392.Jan 15 2020, 4:33 PM
flaub marked an inline comment as done.
  • CR changes

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

From a cursory glance this looks good, I'd use the capital name for enums that conflict with C/C++ keywords.

I'm also very +1 on mirroring LLVM and staying in sync with https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/AtomicOrdering.h#L81.
It makes sense that MLIR would be exactly as "wrong" as LLVM in unsurprising ways.
I feel that developers of frontend X should not have to go read the C++ standard or papers about C++ and Java to understand why there is a surprising mismatch between LLVM and MLIR.

But it seems this would be a good topic to bring to the LLVM community as a whole and then MLIR can just follow?

ftynse added inline comments.Jan 16 2020, 8:26 AM
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1448

result.attributes is literally a vector of named attributes (https://github.com/llvm/llvm-project/blob/e19188af0a2690f222db7d8b866be0afef7b3da0/mlir/include/mlir/IR/OperationSupport.h#L267), fully redundant with the vector attrs you declared locally.

If you were able to do attrs[0].second = <..>, you can also do result.attributes[0].second = <..>

mlir/test/Dialect/LLVMIR/roundtrip.mlir
223

Sorry for not noticing earlier, but please provide the tests for parser error messages in mlir/test/Dialect/LLVMIR/invalid.mlir as well.

mlir/test/Target/llvmir.mlir
1074

I would test each bin_op and each ordering at least once. It's mechanical but guards against subtle syntactic changes.

mehdi_amini added inline comments.Jan 16 2020, 8:46 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751

Could you call this one relaxed, so it matches C++, instead of the LLVM IR name?

@jfb : could we rename the LLVM IR name to align everyone on the C++ name?

jfb added a subscriber: reames.Jan 16 2020, 9:44 AM
jfb added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
751

@jfb : could we rename the LLVM IR name to align everyone on the C++ name?

I think that would make sense, as long as @reames concurs. It's always seemed to be a minor mistake to me, it adds confusion when handling atomics inside of LLVM IR, and it would be a shame for a new IR to mimic it for consistency's sake.

That being said, it's by far not the most confusing thing in LLVM :)

flaub updated this revision to Diff 238603.Jan 16 2020, 1:43 PM
flaub marked 3 inline comments as done.
  • CR changes
flaub added a comment.Jan 16 2020, 1:43 PM

More CR changes incoming

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

@ftynse I believe this is ready to go, anything else you'd like?

Also, I don't yet have committer access, so I'll need some help getting it landed.

jfb requested changes to this revision.Jan 17 2020, 10:46 AM

@ftynse I believe this is ready to go, anything else you'd like?

Also, I don't yet have committer access, so I'll need some help getting it landed.

Please resolve the question I asked, either here or with a follow-up plan. @mehdi_amini's suggestion was good.

This revision now requires changes to proceed.Jan 17 2020, 10:46 AM

I don't think this commit should be about making changes to LLVM IR. I can do that in a followup, but this commit should be in sync with whatever LLVM IR is today. There seems to be consensus with this approach.

flaub added a comment.EditedJan 17 2020, 11:07 AM

@jfb What process do I need to follow to ensure that any changes I make to LLVM IR doesn't break the world? This would include updates to the documentation. I'm new here and I'm not sure which tests I should focus on and where the docs live.

jfb added a comment.Jan 17 2020, 11:09 AM

I don't think this commit should be about making changes to LLVM IR. I can do that in a followup, but this commit should be in sync with whatever LLVM IR is today. There seems to be consensus with this approach.

Agreed this shouldn't make changes to LLVM IR. However, I'd like to understand whether:

  1. a change would occur here first (in this patch, to MLIR), and then in LLVM IR (and who would lead this)
  2. in MLIR and LLVM IR at the same time, as a follow-up, and who would lead this
  3. never

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR. This is so minor, I'm baffled by the pushback. You don't have consensus as evidenced by this discussion, and unwillingness to engage in such a basic question is super worrying. Please don't copy LLVM IR and instead understand why it's designed in a certain manner and what mistakes (however minor) it might contain. I'm not blocking this patch indefinitely, I'm asking you to answer these questions before proceeding. That should take a few minutes.

Changing LLVM IR should _not_ be in this commit, it's semantically different and requires separate discussion with relevant people involved. I also don't see a compelling reason to block this from landing until LLVM IR changes (or decides not to). LLVM dialect models LLVM IR in its current state, whatever the state is.

I'm happy to do whatever the community wants to do. I was under the impression from @ftynse that we shouldn't be changing the MLIR IR, that it should match exactly whatever the LLVM IR is. It's unclear to me what the ramifications of making such a change in LLVM IR are, so I don't feel qualified at this time to make such a change. But, I could certainly take some guidance on this. (this is where my pushback is coming from, sorry if this came off the wrong way).

To clarify further, LLVM dialect in MLIR is not a new IR, it's essentially a representation of LLVM IR using MLIR. It's supposed to minimize the cost of translation from MLIR to LLVM IR. While it does diverge from LLVM IR proper, it does so for modeling reasons: there's no first-class constants in MLIR, neither are there phi nodes. Changing the name of an enumerand locally to the LLVM dialect will increase the cost of translation, which goes against the goal of this dialect. That being said, I do agree that we should use this opportunity to reflect on LLVM IR design and update it following the proper process.

In D72741#1826956, @jfb wrote:

@ftynse I believe this is ready to go, anything else you'd like?

Also, I don't yet have committer access, so I'll need some help getting it landed.

Please resolve the question I asked, either here or with a follow-up plan. @mehdi_amini's suggestion was good.

I wasn't intending to block this revision, I am aligned with @ftynse and @nicolasvasilache that the LLVM dialect in MLIR is intended to mirror as close as possible LLVM IR and so aligning the naming makes sense to me.

I am very supportive to take the opportunity of revisions like the current one to surface issues with LLVM and work towards fixing them, I don't think we need to block the progress on the work on completing the MLIR<->LLVM mapping though.
I was following up on your question with this mindset.

In D72741#1827039, @jfb wrote:

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR.

Maybe what is causing confusion is that we also have the MLIR standard dialect, which is more of a new IR itself. We are actually quite careful about things going in the standard dialect and it isn't bound by LLVM IR (other that we try to think about how to map down to LLVM).

Another way to put it is that the LLVM Dialect is only a layer intended to be hiding the LLVM IR Builders: while in MLIR you lower down to the LLVM Dialect instead of emitting directly LLVM IR. The reason is that this composes better (you can lower progressively to LLVM and not as a one shot conversion). So the LLVM dialect "embeds" the LLVM IR inside MLIR.

jfb accepted this revision.Jan 17 2020, 11:55 AM
In D72741#1827039, @jfb wrote:

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR.

Maybe what is causing confusion is that we also have the MLIR standard dialect, which is more of a new IR itself. We are actually quite careful about things going in the standard dialect and it isn't bound by LLVM IR (other that we try to think about how to map down to LLVM).

Another way to put it is that the LLVM Dialect is only a layer intended to be hiding the LLVM IR Builders: while in MLIR you lower down to the LLVM Dialect instead of emitting directly LLVM IR. The reason is that this composes better (you can lower progressively to LLVM and not as a one shot conversion). So the LLVM dialect "embeds" the LLVM IR inside MLIR.

@flaub and I talked over Discord, and I think we're on the same page! Thanks Mehdi as well. Let's move this forward, and have the naming discussion separately.

tra resigned from this revision.Jan 17 2020, 12:01 PM

Oops. Herald rule misfired.

ftynse accepted this revision.Jan 17 2020, 12:06 PM
This revision is now accepted and ready to land.Jan 17 2020, 12:06 PM
ftynse added a subscriber: tra.Jan 17 2020, 12:06 PM

@tra, your rule adds you as blocking on all of MLIR :)

This revision was automatically updated to reflect the committed changes.

Thanks everyone!

tra added a comment.Jan 17 2020, 12:27 PM

@tra, your rule adds you as blocking on all of MLIR :)

Sorry about the noise. Should be fixed now. I didn't realize that 'blocking reviewer' was the default action on herald rules.