This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add GlobalOp, GlobalLoadConstOp to ml_program.
ClosedPublic

Authored by stellaraccident on Apr 22 2022, 8:05 PM.

Details

Summary

The approach I took was to define a dialect 'extern' attribute that a GlobalOp can take as a value to signify external linkage. I think this approach should compose well and should also work with wherever the OpaqueElements work goes in the future (since that is just another kind of attribute). I special cased the GlobalOp parser/printer for this case because it is significantly easier on the eyes.

In the discussion, Jeff Niu had proposed an alternative syntax for GlobalOp that I ended up not taking. I did try to implement it but a) I don't think it made anything easier to read in the common case, and b) it made the parsing/printing logic a lot more complicated (I think I would need a completely custom parser/printer to do it well). Please have a look at the common cases where the global type and initial value type match: I don't think how I have it is too bad. The less common cases seem ok to me.

I chose to only implement the direct, constant load op since that is non side effecting and there was still discussion pending on that.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2022, 8:05 PM
stellaraccident requested review of this revision.Apr 22 2022, 8:05 PM
rriddle added inline comments.Apr 22 2022, 8:14 PM
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
37
42

The mnemonic is generally passed to MLProgram_Attr, can we follow that style here as well?

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramBase.td
34

nit: Newline before this. Alternatively, just set useDefaultAttributePrinterParser to 1.

https://mlir.llvm.org/docs/DefiningDialects/#default-attributetype-parsers-and-printers

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
121–132

Please use a proper mlir code block.

143

declararative

Also, I don't think we will add anything builtin for this. The assembly format is not intended to have non-generic things inside of it.

171

Same here.

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

Please include the generated tablegen file like with operations, we shouldn't name these directly.

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

Drop trivial braces here.

94

Drop trivial braces here and below.

111

Drop trivial braces here and throughout the commit.

126–142

You should use the SymbolUserOpInterface trait for verifying symbol uses.

rriddle added inline comments.Apr 22 2022, 8:16 PM
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
143

I added some thoughts to the issue.

jpienaar added inline comments.Apr 24 2022, 8:48 AM
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
18

This is already default of AttrDef, lets remove until its actually needed to specialize on (which isn't too common, same is true for traits but that is more likely to change quickly so up to in terms of removing that here too)

27

OpaqueExtern - this is not just extern, this is opaque too. One can have an extern that behaves in all ways like the value it represents but is just external to the context. This explicitly doesn't. It doesn't allow for regular interaction in folding nor does it support reproducers (so error case reproducers are no longer self-contained if one uses this attribute). This is actually beyond opaque even (one can have one that doesn't interact in folding but is still reproducer friendly), NrOpaqueExtern. Although for that one could introduce optional param to differentiate between OpaqueExtern kinds.

(error reproducers are in particular very important to me and has been a pain point for many years in systems where they don't exist, opaque part is more about being clear in objective & usage of attribute; having an extern that just allows for cheaper interactions within a larger system, rather than using this in a offline tool, will also be interesting)

27

s/[]// as default

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

This syntax reads opposite to what I'd expect (and even feels contradictory with one in #123). Could we iterate on this a bit? Something like

ml_program.global mutable @foobar {#extern : tensor<4xi32>} : tensor<?xi32>

or

ml_program.global mutable @foobar init = {#extern : tensor<4xi32>} : tensor<?xi32>

or

ml_program.global mutable @foobar(#extern : tensor<4xi32>) : tensor<?xi32>

all feel a bit more natural to me

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

This is not safe in general. Use verifySymbolUses instead.

Mogball added inline comments.Apr 24 2022, 11:57 AM
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
129

The last version is the syntax I proposed (in line with llvm.global)

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

I'll rework it. I did try it the way requested but was having trouble with the declarative assembly. Now that it is using a custom block, should be able to do it that way.

I don't see the advantage of the alternative suggested but also don't have a strong opinion.

stellaraccident marked 11 inline comments as done.

Address NFC comments.

stellaraccident marked an inline comment as done.May 1 2022, 2:59 PM
stellaraccident added inline comments.
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
18

Thanks. I had a significant challenge finding good examples in tree to copy.

27

I think we may be talking about two different things: you are talking about internal/external to a context, whereas this is indicating external linkage in the traditional sense. I get where you are coming from and can adapt the spelling but just want to be sure we are talking about the same thing.

42

I'm trying to grok what you are asking. I only found a few examples in tree (Test Dialect, Builtin Attributes and Sparse) and I think they are all done as I have done here? Let me know and happy to fix.

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramBase.td
34

Added newline. Setting useDefaultAttributePrinterParser to 1 does not seem to work on its own. I get an error:

In file included from /home/stella/src/llvm-project/mlir/lib/Dialect/MLProgram/IR/MLProgramDialect.cpp:22:
tools/mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.cpp.inc:99:37: error: out-of-line definition of 'parseAttribute' does not match any declaration in 'mlir::ml_program::MLProgramDialect'
::mlir::Attribute MLProgramDialect::parseAttribute(::mlir::DialectAsmParser &parser,
                                    ^~~~~~~~~~~~~~
tools/mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.cpp.inc:117:24: error: out-of-line definition of 'printAttribute' does not match any declaration in 'mlir::ml_program::MLProgramDialect'
void MLProgramDialect::printAttribute(::mlir::Attribute attr,

Maybe there is some missing emission of declarations for that switch? It seems like the generated cpp file should mate with the generated header.

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

Removed TODO.

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

Ah, thanks - I recall going and looking for that thinking it must be there, but I think I must have been looking in the generated header file, not the cpp file (since GET_ATTRDEF_LIST is clearly at the top of the cpp file).

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

Sorry about that: My eyes have become uncalibrated to this quirk of LLVM style. Re-read the guidelines and I think I've fixed them all.

126–142

Thank you! I had long wondered the right way to do that and avoid multiple linear scans. Filing an issue in the downstream from which I picked up this bad habbit.

stellaraccident marked an inline comment as done.

Change global asm format.

Update tests.

Asm format updated - PTAL

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

Ok, so trying the obvious:

let assemblyFormat = [{
  custom<SymbolVisibility>($sym_visibility)
  (`mutable` $is_mutable^)?
  $sym_name
  (`(` $value^ `)`)?
  `:`
  $type
  attr-dict
}];

Yields an error:

error: format ambiguity caused by `:` literal found after attribute `value` which does not have a buildable type
    custom<SymbolVisibility>($sym_visibility)

I don't quite understand why this is ambiguous.

129

Ok, I simplified and implemented the last option.

I'm not thrilled with how the #extern attribute prints (it currently prints as an alias) as this is short and load bearing to how to interpret the global (I think it should be inline). If inline, I'd also like it to print without the dialect prefix. I recall there being various discussions about this, but I'm having trouble figuring out the right way to hold it to get what I want. Also, this may be good enough to get started.

Mogball added inline comments.May 5 2022, 2:39 PM
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
41

The self type should not be in the assembly format.

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

That sounds like a bug in the assembly format. It should not be ambiguous because $value is followed by a literal.

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

If you want to drop the prefix, you can specially check for ExternAttr in both the printer and parser.

mlir/test/Dialect/MLProgram/ops.mlir
29

The type on extern goes away?

Sorry for the delay, was semi-OOO.

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
42
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramBase.td
34

Weird, can you file a bug for this?

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

Can this be dropped? Or is this comment meant for something?

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

Missing comment here?

57

I'll look into moving this somewhere shared in a followup.

111

(No worries at all)

stellaraccident marked 5 inline comments as done.May 7 2022, 1:50 PM
stellaraccident added inline comments.
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
41

ODS bug? Changing to let assemblyFormat = ""; produces this error:

error: format is missing reference to parameter: type

^
/home/stella/src/llvm-project/mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td:25:1: note: in custom assembly format for this operation
def MLProgram_ExternAttr : MLProgram_Attr<"Extern"> {
^
/home/stella/src/llvm-project/mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td:25:1: error: failed to parse assembly format
def MLProgram_ExternAttr : MLProgram_Attr<"Extern"> {

Side note: the lack of actual use of this stuff upstream has made all of the ODS work in this patch incredibly painful and time consuming. Can you point me to a test case that shows proper usage? Otherwise, I can file an issue.

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramBase.td
34
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramOps.td
152

Sorry, that was the result of me trying to figure out what was wrong with the declarative assembly generator. Let me capture in a bug/test op and will drop here.

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

I don't care deeply enough about it in this patch to change it. I may followup with something else, but I've spent quite a bit of time tracking through getting this to print/parse and interlocking issues. I would like to checkpoint as I have it here.

mlir/test/Dialect/MLProgram/ops.mlir
29

It is carried in the assembly format. I could not figure out how to get it to print as I expected: #ml_program.extern : tensor<4xi32>. Please point me at a test case that shows proper usage (all I could find were builtins that were all weird in one way or another). Or I can create a test case/issue and iterate on it in a followup.

stellaraccident marked 3 inline comments as done.

Comments.

PTAL.

As a side note, this is possibly the single most painful patch I've made. I am not new to defining ops or using ODS, but trying to do it "right" (i.e. in downstreams, we have sometimes just open coded parsers/printers and such) cost hours, hit multiple issues and it was really hard to find non-builtin examples of things done right (the builtins examples help, but they are often "weird" in various ways that it was hard to see through). I did a lot of spelunking through downstreams, each of which represents a point in time and may not have done it right...

I'm not so much complaining as saying that I think it is important that we get more corner case usage of ODS upstream. This dialect should help but we should have more production uses. I've filed this issue: https://github.com/llvm/llvm-project/issues/55322 and will file another (couple) for declarative assembly snags.

Yeah that's... unfortunate to hear. ODS can be a bag of knives sometimes. I'll take a look at the bugs.

jpienaar added inline comments.May 12 2022, 12:10 PM
mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
27

You are correct (i was thinking of extern linkage too, but I wrote here exclusively wrt context indeed). A difference with extern linkage as in C (I think) is that there isn't a generic resolution mechanism or LTO that can be generally done. And that is, as far as I understand, a desirable and required property here: nothing but a specific implementation of a target that can do something with this attribute. It is meant to represent a placeholder/be opaque for all others. No freezing'esqu behavior should happen "accidentally". Is that correct? (And then of course question about leaving space for something with external linkage but generic way of loading that allows for something like generic LTO)

mlir/test/Dialect/MLProgram/ops.mlir
29

I'm almost back, and Jeff is probably back from EuroLLVM then too :-) Then could try and find example here, this form is readable though.

(This part is part fun, part bike shedding to make sure it is easy for folks, especially as this crosses framework boundaries erring on the side of more verbose but trivial to interpret has value. Can help as it sounds like this hasn't been as much fun)

Yeah that's... unfortunate to hear. ODS can be a bag of knives sometimes. I'll take a look at the bugs.

Ok, you self-type fix worked. Thanks for that!

Rebase and adapt to self-type attribute fix.

Jacques/Jeff, please take a look if you have a chance. I see there is a patch pending to fix the attribute parser declarations, but that will be a minor adaptation. I think the IR format is good enough as I have it here.

Update bazel files.

jpienaar accepted this revision.May 18 2022, 4:15 AM

Looks good/we can refine in follow ups, thanks!

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
27

May have forgotten to upload (removing trait default)

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

I dont think this function does this one (e.g., it always does colon type at moment). Also s/global/op/

This revision is now accepted and ready to land.May 18 2022, 4:15 AM
stellaraccident marked an inline comment as done.

Comments

mlir/include/mlir/Dialect/MLProgram/IR/MLProgramAttributes.td
27

I think it is removed? Let me know if I'm seeing something different than you.

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

Thanks. Stale comment. Fixed.

This revision was landed with ongoing or failed builds.May 18 2022, 11:09 PM
This revision was automatically updated to reflect the committed changes.