This is an archive of the discontinued LLVM Phabricator instance.

Make namespace handling uniform across dialect backends.
ClosedPublic

Authored by fedelebron on Aug 28 2020, 1:10 PM.

Details

Summary

Now backends spell out which namespace they want to be in, instead of relying on
clients #including them inside already-opened namespaces. This also means that
cppNamespaces should be fully qualified, and there's no implicit "::mlir::"
prepended to them anymore.

Depends On D86810

Diff Detail

Event Timeline

fedelebron created this revision.Aug 28 2020, 1:10 PM
fedelebron requested review of this revision.Aug 28 2020, 1:10 PM

Rebasing onto head, fixing build errors.

rriddle requested changes to this revision.Aug 28 2020, 3:03 PM

Thanks.

mlir/include/mlir/Dialect/GPU/GPUBase.td
25

nit: Move this up one line.

mlir/include/mlir/Dialect/GPU/GPUOps.td
207 ↗(On Diff #288712)

Why this change? Seems unrelated.

273 ↗(On Diff #288712)

I don't understand why these changes are necessary.

mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
337–338

These comments need to be reflowed.

mlir/tools/mlir-tblgen/DialectGen.cpp
157

nit: / -> .

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2170

This seems like something that could be refactored into a general utility that all of the backends could share.

2176

nit: Drop trivial spaces here and below.

2185

nit: Newline before here.

2187

nit: Drop the llvm::.

This revision now requires changes to proceed.Aug 28 2020, 3:03 PM
fedelebron marked 3 inline comments as done.

Resolving review comments.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
511

Does this use of Twine look safe? I've very little experience with that type.

2170

Where do such utilities go?

2176

Sorry I don't follow. What trivial spaces?

mehdi_amini added inline comments.Aug 28 2020, 4:55 PM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2176

Braces maybe? (on the loop right below)

Please run clang-format, seems like there are a few issues sprinkled about.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2176

Yep, meant braces.

(Sorry, I don't always type what I'm thinking for some reason)

Fixing some targets.

fedelebron marked 7 inline comments as done.

Lint fix.

fedelebron marked an inline comment as done.Sep 10 2020, 12:08 PM

I think I addressed all comments, PTAL?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2185

Not sure where the newline would go, but also not sure I'm reading this UI correctly as to where you wanted a newline. Marking this as tentatively resolved since I moved NamespaceEmitter away from here.

Fixing more targets.

Anything more to do here or can this be merged?

Lint fixes.

mehdi_amini accepted this revision.Sep 14 2020, 1:31 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 14 2020, 1:33 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.