Page MenuHomePhabricator

[MLIR][GPU] Expose GpuParallelLoopMapping as non-test pass.
ClosedPublic

Authored by csigg on May 23 2022, 5:55 AM.

Diff Detail

Event Timeline

csigg created this revision.May 23 2022, 5:55 AM
csigg requested review of this revision.May 23 2022, 5:55 AM
csigg updated this revision to Diff 431352.May 23 2022, 6:10 AM

Comment fix.

herhut accepted this revision.May 25 2022, 8:18 AM

If it useful beyond testing, I don't see why it should not be a regular pass.

This revision is now accepted and ready to land.May 25 2022, 8:18 AM
bondhugula accepted this revision.May 25 2022, 10:02 AM

LGTM - should have definitely been a regular pass. Some minor comments.

mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp
146

I think you no longer need this macro.

155

You are already in the mlir namespace.

csigg updated this revision to Diff 432079.May 25 2022, 12:27 PM
csigg marked an inline comment as done.

Clean up namespacing.

csigg added inline comments.May 25 2022, 12:28 PM
mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp
146

I think you still need it for passes with internal linkage: https://mlir.llvm.org/doxygen/classmlir_1_1TypeID.html#details

155

It's a namespace-qualified definition, so the return types need the namespace qualifier as well.

I did a pass to make the namespace use more consistent with the rest of mlir.

rriddle added inline comments.May 25 2022, 12:37 PM
mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp
146

You don't need it for passes defined in ODS, it's already handled for you.

bondhugula added inline comments.May 25 2022, 5:32 PM
mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp
26

It was fine the way it was earlier -- we shouldn't be enclosing all of this in the mlir namespace, but instead just be defining the methods as LogicalResult mlir::gpu::.... See https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
It allows one to catch build errors early.

59

Again, these should just go into the anonymous namespace.

csigg updated this revision to Diff 432839.May 29 2022, 11:32 PM

Remove TYPE_ID definition.

csigg added inline comments.May 29 2022, 11:34 PM
mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp
26

These were defined in the mlir::gpu namespace before, which goes against the coding standard you linked. I moved the definition to the mlir namespace and qualified it with gpu to get early build errors. Fully qualifying them with`mlir::gpu` doesn't give us anything extra.

59

I put it in the mlir::gpu namespace because MappingLevel is a sufficiently generic term that there is risk that someone will define it in the mlir or global namespace.

146

Thanks, I didn't know.

rriddle added inline comments.May 29 2022, 11:38 PM
mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp
26

Please drop these in favor of using namespace mlir/etc. and anonymous namespaces.

This revision was automatically updated to reflect the committed changes.