Page MenuHomePhabricator

[MLIR] Introduce std.global_memref and std.get_global_memref operations.
ClosedPublic

Authored by jurahul on Oct 28 2020, 12:44 PM.

Details

Summary
  • Add standard dialect operations to define global variables with memref types and to retrieve the memref for to a named global variable
  • Extend unit tests to test verification for these operations.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
silvas added inline comments.Oct 28 2020, 3:28 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2015

Can you provide some description here about the intent that the memref descriptor itself is not considered mutable, to simplify global alias analysis.

2026

can provide examples with some commentary for:

  1. external global memref.
  2. uninitialized global memref.
2038

also add isUnintialized()?

2051

for clarity: "writing to the result memref (such as through a std.store op)"

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2157

nit: newline after this banner comment

2171

is ::mlir needed?

2192

nit: Capitalize first letter

Also, I think a description equivalent to this comment belongs in the let description = on the ODS side. (and then you can remove this comment if desired)

2217

nit: newline after this banner comment

2221

These lookups can be expensive since there is no symbol table build here.

I think the preferred pattern these days is to implement SymbolUserOpInterface which allows for efficient verification https://mlir.llvm.org/docs/Interfaces/#symbolinterfaces

mlir/test/Dialect/Standard/invalid.mlir
268

should private visibility + missing initial_value be a verifier error?

rriddle added inline comments.Oct 28 2020, 5:31 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2158

Can you just use the declarative syntax for this? You just need to use SymbolNameAttr for the sym_name attribute and it will format it the way you want.

jurahul updated this revision to Diff 301489.Oct 28 2020, 5:34 PM

Address review comments.

jurahul marked 11 inline comments as done.Oct 28 2020, 5:40 PM
jurahul added inline comments.Oct 28 2020, 5:44 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2158

I did look into that. For Symbols the documentation says that sym_name should be a string attribute. And with that the declarative syntax would then format it as just a string. I also tried adding @ to the syntax but that's not one of the allowed literal, so I fell back to manual print/parse.

Let me try the SymbolNameAttr.

jurahul updated this revision to Diff 301493.Oct 28 2020, 5:50 PM

Use declarative syntax for global_memref as well.

jurahul marked an inline comment as done.Oct 28 2020, 5:51 PM
jurahul added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2158

SymbolNameAttr works. Thanks!

silvas accepted this revision.Oct 28 2020, 6:58 PM

Thanks Rahul, this looks excellent.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2026

I think the terminology "memref pointing to the global variable" is a bit confusing. Maybe "retrieve the memref descriptor held in this std.global_memref op"?

2084

Can we tighten this to AnyStaticShapeMemRef? I know that the SymbolUserOpInterface implementation does the all the necessary checking, but this field also serves a documentation purpose (e.g. people reading the .td file or in generated documentation).

I also notice that you have some code in verify() that could be removed by this.

This revision is now accepted and ready to land.Oct 28 2020, 6:58 PM
mehdi_amini added inline comments.Oct 28 2020, 8:27 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2022–2023

Can you specify the meaning of this? (for example mention that a store to a constant global is UB)

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2162

In general, I like to be more explicit in error messages to avoid having to spawn a debugger (or add more print on demand):

op.emitOpError() << "type should be static shaped memref, but got " << op.type();

(same below)

2189

Actually I'm not entirely convinced that the *declaration* should be public, for example it wouldn't be DCE if it is the case.

mlir/test/Dialect/Standard/ops.mlir
97

I wonder if we could do print more "pretty":

global_memref @foo : memref<2x2xf32>

global_memref @foo : memref<2x2xf32> = [0.0, 1.0]

global_memref constant @foo : memref<2x2xf32> = [0.0, 1.0]

global_memref private constant @foo : memref<2x2xf32> = uninitialized
jurahul marked 5 inline comments as done.Oct 29 2020, 10:32 AM
jurahul added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2026

The std.global_memref op itself does not hold the memref in my mental model, i.e., its contents are not the memref itself but the buffer. I changed it to "retrieve the memref for the global variable". In my mental model, std.get_global_memref will actually construct the memref by packing say the backing buffer pointer and whatever other things are needed to construct the descriptor.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2189

Looking into other such cases, I did not see any verification like this for either FuncOp or LLVM::GlobalOp.

I think its pretty clear that a definition can have all 3 allowed visibility attributes (public, private, and nested). When it comes to declarations, the intent of a declaration to "import" a symbol that is defined external to the IR. Such an import is legal only if the definition of the symbol in the defining IR is public. That's a linker level check which we cannot do here. The declaration itself can then define the visibility of this "import" in the current IR. Private visibility means that only get_global_memrefs local to the current operation can reference the declaration. Nested means that any operation in the current IR can reference the declared GV. It seems public "import" is problematic since if external IR wants to reference the GV, that reference will be to the public definition of the symbol in the defining IR, and not a declaration in this IR (which does not define the symbol). So declarations themselves cannot have public visibility. So it seems what we want is:

if (op.isExternal() && op.isPublic()) {
    "global variable declarations cannot have public visibility"
}

WDTY? That way, unreferenced GV's will get eliminated correctly by SymbolDCE.

mlir/test/Dialect/Standard/ops.mlir
97

I don't know if this is possible with the declarative syntax (specifically sym_visibility is optional attribute). I'll try custom printer/parser here.

jurahul marked an inline comment as done.Oct 29 2020, 4:13 PM
jurahul added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2189

Just to be super clear: That way, unreferenced GV declarations will get eliminated correctly by SymbolDCE.

mehdi_amini added inline comments.Oct 29 2020, 4:20 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2189

WDTY? That way, unreferenced GV's will get eliminated correctly by SymbolDCE.

Yes this is exactly what I had in mind in term of reasoning.

jurahul updated this revision to Diff 301807.Oct 29 2020, 8:43 PM

Add pretty printing/parsing for global_memrefs. Also removed the external vs visibility checks.

jurahul marked 2 inline comments as done.Oct 29 2020, 8:48 PM
jurahul added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2189

Based on offline discussion, we decided to leave this as a TODO and start a discussion about this on the MLIR discourse since Functions and other symbol operations also likely need such a check.

mlir/test/Dialect/Standard/ops.mlir
97

I have implemented the pretty format using a custom parser and printer. It's not exactly like above, it still prints dense<[...]> but elides the tensor type as that can be derived from the memref type. To be able to do this (hide the tensor type), I had to repeat some of the checks in the Op verification (like verifying that the type is a static shaped memref) so that I can call the getTensorTypeFromMemRefType on a valid memref type during parsing.

mehdi_amini accepted this revision.Oct 29 2020, 10:03 PM

LGTM overall, nice syntax! Thanks

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2258

Can you get coverage for the error cases?

2312

Nit: remove trivial braces (and below)

mlir/test/Dialect/Standard/ops.mlir
97

Nice result!

rriddle added inline comments.Oct 29 2020, 11:20 PM
mlir/test/Dialect/Standard/ops.mlir
97

Can you please move back to the declarative form and use a custom directive for the initializer. A majority of the syntax can be declarative.

https://mlir.llvm.org/docs/OpDefinitions/#custom-directives

rriddle requested changes to this revision.Oct 29 2020, 11:25 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2055

The hasValue here is unnecessary.

mlir/include/mlir/IR/SymbolTable.h
76 ↗(On Diff #301807)

Why are these necessary? The visibility attribute is already a StringAttr. If you use the declarative assembly form these shouldn't be necessary.

This revision now requires changes to proceed.Oct 29 2020, 11:25 PM
jurahul marked an inline comment as done.Oct 30 2020, 9:27 AM
jurahul added inline comments.
mlir/test/Dialect/Standard/ops.mlir
97

Thanks. If this can still be supported with a declarative form, I am all for it. Let me try that.

jurahul updated this revision to Diff 301988.Oct 30 2020, 12:18 PM
jurahul marked 3 inline comments as done.

Use declarative syntax with custom directives.

jurahul marked 3 inline comments as done.Oct 30 2020, 12:19 PM
jurahul added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2258

When we switch to declarative assembly syntax, it seems there is no equivalent check. Seems something which needs to be added to the code generated by ODS framework? I'll file a bug for this.

jurahul marked an inline comment as done.Oct 30 2020, 12:20 PM
jurahul added inline comments.Oct 30 2020, 12:26 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2258

Windows build failure is an unrelated issue in RISC-V backend. Linux failure is clang-tidy warnings for the parse helper functions to use camelCase variable names for arguments. I think it makes sense to keep these names matching the names in the operations.

rriddle added inline comments.Oct 30 2020, 2:40 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2165

Why is this necessary? I would expect you can just print the string attribute normally.

2182

You can format this in the declarative syntax like:

(`constant` $constant)?

There is builtin support for formatting UnitAttrs this way, i.e. the UnitAttr won't be in the textual format.

silvas added inline comments.Oct 30 2020, 2:51 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2182

I think it is with a ^

(`constant` $constant^)?

https://mlir.llvm.org/docs/OpDefinitions/#unit-attributes

jurahul updated this revision to Diff 302055.Oct 30 2020, 6:16 PM
jurahul marked 2 inline comments as done.

Review feedback

jurahul added inline comments.Oct 30 2020, 6:17 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2165

StringAttr would print normally as a string with quotes I think, whereas we want it to be printed without quotes. But I think that might be ok if it lets us the declarative syntax more. I tried that and it needed some fixes to the parser, but seems to be working.

2182

Thanks. I figured that one out. Also, since the sym_visibility is optional, that also needed the anchor in the declarative syntax.

jurahul marked an inline comment as done.Oct 30 2020, 6:17 PM
jurahul added inline comments.Nov 2 2020, 9:18 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2058

I tried $sym_name : $type here so that I could use some more generated code, but looks like the custom syntax does not allow binding a value twice, and I do need the parsed type in custom<GlobalMemrefOpTypeAndInitialValue>. So I think this is the best we can squeeze out of the custom syntax.

rriddle added inline comments.Nov 2 2020, 10:13 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2058

Can you use type_ref for that?

jurahul marked an inline comment as done.Nov 2 2020, 11:05 AM
jurahul added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2058

The custom syntax does not allow it on attributes, I get: "invalid argument to 'type' directive". ODS doc says:

type_ref ( input )
input must be either an operand or result variable , the operands directive, or the results directive.

rriddle accepted this revision.Nov 2 2020, 11:09 AM

Thanks Rahul, looks great.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2058

Ah yeah, forgot this was a TypeAttr.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2162

This variable should be camelCase.

2175

Same here.

2188

nit: Can you switch to early exit here?

This revision is now accepted and ready to land.Nov 2 2020, 11:09 AM
jurahul updated this revision to Diff 302352.Nov 2 2020, 11:21 AM
jurahul marked an inline comment as done.

Address final comments

jurahul marked 3 inline comments as done.Nov 2 2020, 11:22 AM
This revision was landed with ongoing or failed builds.Nov 2 2020, 1:43 PM
This revision was automatically updated to reflect the committed changes.