This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add global_load and global_store ops to ml_program.
ClosedPublic

Authored by stellaraccident on May 23 2022, 11:42 AM.

Details

Summary
  • Adds simple, non-atomic, non-volatile, non-synchronized direct load/store ops.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.May 23 2022, 11:42 AM

*breathes a sigh of relief that no ODS bugs were stepped on*

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
162

It is possible to specify memory effects on symbol reference resources specifically, but ODS doesn't generate that code for you. Doing so should provide the benefit of being able to reorder/CSE certain reads and writes.

184

I don't have context on this but does disallowing nested symbol references make sense for this dialect?

291

Ditto with the symbol reference resource.

mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
187

add braces

238

add braces

242

add braces

*breathes a sigh of relief that no ODS bugs were stepped on*

Haha - yes, this one was much easier! Thank you for all of your help on the previous patch.

Since the conversation on design is still happening on discourse, I'll leave this until that resolves before applying your suggestions. Thank you for the review!

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
162

Oh, thank you for the tip -- I had wondered about this in the past! I can go reading, but do you have any reference to look at?

Mogball added inline comments.May 25 2022, 10:20 AM
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
162

Oh, ODS does the work for you. I had never seen examples in TableGen before :P

stellaraccident marked 8 inline comments as done.May 27 2022, 5:18 PM
stellaraccident added inline comments.
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
162

This is nice. Thanks!

184

Not a good reason. Changed.

mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
187

I am still confused on when to brace and when to not. But taking your word for it.

stellaraccident marked 3 inline comments as done.

Comments

Nice, mostly looks good, clarification question (and sorry I jumped to the code while you may have been making update in discourse/my mind is clear after long weekend so may be asking things already discussed)

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
414

Is this a pure compilation side type (e.g., same as shape.witness) or does it have a runtime value too?

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramTypes.td
19

You can elide this (replace { ... } with just ; and it behaves the same).

24

You can just elide description if nothing useful :)

If this is meant to be compile time only, this could be useful part of description though.

mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
24

ah token and chains ... Mmm, bike shedding: could we have this be something like "after" instead? I'm not even sure if you need to have the mapping there for the tokens produced, that is captured in the type isn't it? Oh but not in the pretty print type. So it would need to be something like

%token4, %token5 = ml_program.global_store @global_mutable_undef = %0 : tensor<?xi32> after (%token1, %token3) : (!ml_program.token, !ml_program.token)

if not captured in mapping. But what does it mean for a store to produce multiple tokens? I'm not sure what it means from an ordering point of view. Why not always make it produce one token?

%after_token = ml_program.global_store @global_mutable_undef = %0 after (%token1, %token3) : tensor<?xi32>

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
414

I don't think we care/specify: I have seen implementations that just use this for compile time support, and I have seen implementations that transform/surface such things across boundaries to "externalize" the ordering constraint as needed.

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramTypes.td
24

Oh, I left it there because I meant to write the description. Sorry!

stellaraccident marked 2 inline comments as done.

Comments

mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
24

Ok, made the result be one optional token (I meant to do that but got turned around when undoing the earlier tablegen experiment).

I fiddled with it some more. Here are some examples:

%token1 = ml_program.token
%0, %token2 = ml_program.global_load @global_mutable_undef
    after (none -> !ml_program.token) : tensor<?xi32>
%token3 = ml_program.global_store @global_mutable_undef = %0
    after (%token1, %token2 -> !ml_program.token) : tensor<?xi32>
ml_program.global_store @global_mutable_undef = %0
    after (%token3) : tensor<?xi32>

I don't love all of these but I think they convey the meaning in the expected common cases.

Some considerations:

  • It would be nice if the after clause was at the end, but this actually creates an ambiguity with a dialect called after I think :/
  • I think that having the producing token be optional is right, since it makes it statically clear when this is a "chaining" op vs not. Practically, it also makes the printed form without tokens nice.
  • I'm still not sold on after. Maybe chaining, ordered-by or some such thing?
Mogball added inline comments.May 31 2022, 3:23 PM
mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
24

I'm not crazy about none. If there will be at most one token, what about

%0, %token1 = ml_program.global_load @foo after (%token1) : tensor<...>
%token2 = ml_program.global_store @bar = %0 after(%token1, %token0) : tensor<...>
39

drop braces

mlir/test/Dialect/MLProgram/ops.mlir
45

extra //

Mogball added inline comments.May 31 2022, 3:24 PM
mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
24

My edit didn't go through... it's meant to be after (() -> !ml_program.token) or maybe some other way of indicating the optional token since the type itself isn't needed.

stellaraccident marked an inline comment as done.

Comments.

mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
24

Switched to the '()' means nil form -- have a look. I had thought about some other way to avoid spelling out the return token type (since it is buildable and we don't technically need it). However, I expect that in the not too distant future, for interop, we're going to want to widen the allowable TokenTypes (since existing graph based dialects all have one and there is little practical reason to restrict them, if we can come up with some common API).

39

Ok, I promise I'm trying and I've read this part of the style guide like 15 times -- and missing something. This case and the if directly above seem to satisfy this statement by my reading: "Maintainability is harmed if the body of an if ends with a (directly or indirectly) nested if statement with no else. Braces on the outer if would help to avoid running into a “dangling else” situation."

(ftr - I really don't care much but am trying to comply and get the feel for how this codebase wants it so you all don't need to keep leaving these comments for me :) )

Bazel updates.

jpienaar accepted this revision.Jun 1 2022, 10:54 AM

Looks good thanks!

mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
24

Yes esp if we do "before -> after" then after as keyword is not accurate. I'm good with chaining, ordered-by or other words convenying it is a scheduling constraint.

Re: ambiguity of after at end, if optional & op in dialect called after without return value is a thing. Also good point on potential different return types ... One could have the return type at end (rather than operand type) and avoid ambiguity

ml_program.global_store @global_mutable_undef(%0 : tensor<?xi32>) after(%token3, %token4) : !ml_program.token

and

ml_program.global_store @global_mutable_undef(%0 : tensor<?xi32>) : None

or

ml_program.global_store @global_mutable_undef after(%token3, %token4 : !ml_program.token) = %0 : tensor<?xi32>

But I'm not going to block about any of these, yours reads well enough and we can iterate.
This revision is now accepted and ready to land.Jun 1 2022, 10:54 AM

Tweak based on comments.

Looks good thanks!

I ended up trying to use an "ordered-by" keyword but parseOptionalKeyword silently will not parse (presumably because of the dash). I ended up spelling it "ordering" but am open to change the spelling. Also added some comments to the custom directive functions.

Mogball accepted this revision.Jun 1 2022, 11:27 AM

Just one, otherwise braces LGTM

mlir/lib/Dialect/MLProgram/IR/MLProgramOps.cpp
39

For me, it's been a journey of trial and getting nitted by River :P

It also occasionally changes vs. older parts of the codebase...

53

drop braces.

The rule here is something like nested indents one line each are okay, i.e.

if (foo)
  for (a in b)
    while (bar)
       if (c)
          do_something()

is okay

stellaraccident marked an inline comment as done.

Remove braces.

This revision was landed with ongoing or failed builds.Jun 1 2022, 11:32 AM
This revision was automatically updated to reflect the committed changes.