This is an archive of the discontinued LLVM Phabricator instance.

[mlir][GPUDialect] Implement memory attributions for LaunchOp
ClosedPublic

Authored by fmorac on Apr 7 2023, 1:36 PM.

Details

Summary

Currently memory attributions are not supported for gpu::LaunchOp, this patch implements memory attributions for gpu::LaunchOp and modifies the KernelOutlining pass to make the attributions available in GPUFuncOp.

Diff Detail

Event Timeline

fmorac created this revision.Apr 7 2023, 1:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
fmorac requested review of this revision.Apr 7 2023, 1:36 PM
fmorac updated this revision to Diff 511791.Apr 7 2023, 2:04 PM

Constructed explicitly the TypeRanges in outlineKernelFuncImpl when building gpu::GPUFuncOp.

fmorac updated this revision to Diff 511922.Apr 8 2023, 1:04 PM

Fixed a typo in GPU_LaunchOp description examples.

Ping for review.

mehdi_amini added inline comments.Apr 18 2023, 10:03 AM
mlir/test/Dialect/GPU/outlining.mlir
353

Can you restrict the test to the memory attribution? It's not clear to me that we need to match every operands and other stuff here.

382

Same here: I'm pretty sure we don't need all of these checks

396

Can you clarify how are the checks above related to the attributions?
Maybe some test documentation can be useful here.

fmorac added inline comments.Apr 18 2023, 10:25 AM
mlir/test/Dialect/GPU/outlining.mlir
353

These tests are variants from 2 existing tests in this file, my rationale was that I wanted to proof that adding the attributions doesn't mess with anything already existing. However I'll remove them.

396

Yeah, I'm just realizing tests should be target for the specific feature. The only lines that truly tests are 391 and 403-404. I'll change the tests.

fmorac updated this revision to Diff 514693.Apr 18 2023, 11:01 AM

Removed unnecessary checks from the tests.

fmorac marked 3 inline comments as done.Apr 24 2023, 5:51 AM

Ping for review.

makslevental accepted this revision.Apr 26 2023, 11:38 AM

LGTM

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
380

I don't know if this is the kind of thing they're used for, but a LLVM_DEBUG(llvm::dbgs() << "Couldn't get AddressSpaceAttr for: " << type << "\n")(but then again there's no LLVM_DEBUG in this file at all so maybe such reporting isn't the current convention).

744

You didn't do this but can you change this to say "all of them index types" (which also makes it clearer that the sentence is listing the region args).

780

same here regarding plurals

This revision is now accepted and ready to land.Apr 26 2023, 11:38 AM
fmorac updated this revision to Diff 517263.Apr 26 2023, 12:10 PM

Addressed the typos by changing the phrase to:

all having index type.

fmorac marked 3 inline comments as done.Apr 26 2023, 12:15 PM
fmorac added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
380

All of these functions were already there, I just relocated them to the start of the file. Having said that, it might be a good idea to start introducing this kind of debug annotations.