Page MenuHomePhabricator

[mlir][gpu] Refactor functions for workgroup and private buffer attributions.
ClosedPublic

Authored by whchung on May 6 2020, 10:48 AM.

Details

Summary

Consolidate interfaces adding workgroup and private buffer attributions in GPU
dialect.

Note all private buffer attributions must follow workgroup buffer attributions.

Diff Detail

Event Timeline

whchung created this revision.May 6 2020, 10:48 AM
herhut requested changes to this revision.May 7 2020, 5:03 AM

Thanks for adding this!

mlir/include/mlir/Dialect/GPU/GPUOps.td
221

It is not clear to me why we have two versions of this function. While you have not caused this, could you clean it up and unify them? One could implement addWorkgroupAttribution(ArrayRef<int64_t> shape, Type elementType) by means of BlockArgument addWorkgroupAttribution(Type type).

I see only one use of the former form in MemoryPromotion. Changing that use to the type based one and removing the shape/elementType versions would be best.

Also, both of these could be implemented in the cpp file and only be declared here.

226

Use getNumWorkgroupAttributions here?

This revision now requires changes to proceed.May 7 2020, 5:03 AM
whchung updated this revision to Diff 262647.May 7 2020, 7:31 AM

Address review comments.

whchung marked 3 inline comments as done.May 7 2020, 7:34 AM
whchung added inline comments.
mlir/include/mlir/Dialect/GPU/GPUOps.td
221

@herhut I was somewhat puzzled by the two versions of interfaces too. I've revised the patch so we we consolidate the interfaces. Could you help conduct another round of review? Thanks.

whchung retitled this revision from [mlir][gpu] Add utility functions to add private buffer attributions. to [mlir][gpu] Refactor functions for workgroup and private buffer attributions..May 7 2020, 7:35 AM
whchung edited the summary of this revision. (Show Details)

Thanks for the cleanup.

mlir/include/mlir/Dialect/GPU/GPUOps.td
195

These getters that give access to a range should remain in the header to enable inlining (and maybe avoid materializing the ArrayRef).

228

I don't think this is needed. The contract could simple be that all trailing operands after function arguments and workgroup attributions are private. Storing an extra count does not add benefit. You would then need to verify that arguments + all attributions = operand count.

So let's just drop the extra attribute.

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

This would just be {begin, getBody().front().args_end()}

@whchung Are you still interested in moving this forward?

whchung updated this revision to Diff 264899.May 19 2020, 7:23 AM
whchung marked an inline comment as done.

Address code review comments.

whchung marked 3 inline comments as done.May 19 2020, 7:26 AM

@whchung Are you still interested in moving this forward?

@herhut I just revised the patch to address your review comments. Getting rid of the counter for private allocations is less than ideal in my applications but I've found ways to get around them. Could you give this patch another look? Thanks.

@whchung Are you still interested in moving this forward?

@herhut I just revised the patch to address your review comments. Getting rid of the counter for private allocations is less than ideal in my applications but I've found ways to get around them. Could you give this patch another look? Thanks.

I think you misunderstood my comment. I liked having the getters and interface for private attributions. My point was that you do not need to store a count as an attribute for the private attributions but instead could always compute the number of private attributes via getNumOperands() - getType().getNumInputs() - getNumWorkgroupAttributions(). Sorry for not being clear.

Using that approach should also cover your use cases?

@whchung Are you still interested in moving this forward?

@herhut I just revised the patch to address your review comments. Getting rid of the counter for private allocations is less than ideal in my applications but I've found ways to get around them. Could you give this patch another look? Thanks.

I think you misunderstood my comment. I liked having the getters and interface for private attributions. My point was that you do not need to store a count as an attribute for the private attributions but instead could always compute the number of private attributes via getNumOperands() - getType().getNumInputs() - getNumWorkgroupAttributions(). Sorry for not being clear.

Using that approach should also cover your use cases?

@herhut Yes, that's exactly how I compute the number of private attributes right now in my application.

BTW it seems nowadays pre-merge checks would always fail on Windows due to logic in SPIR-V dialect.

[2020-05-19T14:57:38.942Z] tools\mlir\include\mlir/Dialect/SPIRV/SPIRVSerialization.inc(4334): fatal error C1061: compiler limit: blocks nested too deeply

BTW it seems nowadays pre-merge checks would always fail on Windows due to logic in SPIR-V dialect.

[2020-05-19T14:57:38.942Z] tools\mlir\include\mlir/Dialect/SPIRV/SPIRVSerialization.inc(4334): fatal error C1061: compiler limit: blocks nested too deeply

This should be fixed by patch https://github.com/llvm/llvm-project/commit/d5b1643c74eeae327d85c75fe79fd98edb1014f9

@whchung Are you still interested in moving this forward?

@herhut I just revised the patch to address your review comments. Getting rid of the counter for private allocations is less than ideal in my applications but I've found ways to get around them. Could you give this patch another look? Thanks.

I think you misunderstood my comment. I liked having the getters and interface for private attributions. My point was that you do not need to store a count as an attribute for the private attributions but instead could always compute the number of private attributes via getNumOperands() - getType().getNumInputs() - getNumWorkgroupAttributions(). Sorry for not being clear.

Using that approach should also cover your use cases?

@herhut Yes, that's exactly how I compute the number of private attributes right now in my application.

Why not add a getNumPrivateAttributions to the GPUFunc for this? Then the interface is exactly as before.

With that change, I think this is good to land.

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

I would prefer if new private attributions would also be added at the end of the existing private attributes, rather than at the front. You could just use addArgument for this.

whchung updated this revision to Diff 265256.May 20 2020, 8:16 AM
whchung marked an inline comment as done.

Reinstate getNumPrivateAttributions() in GPUFunc.

Use it in verifier logic, and addPrivateAttribution() logic.

whchung marked an inline comment as done.May 20 2020, 8:18 AM
whchung added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
478

@herhut addArgument might not guard the case addPrivateAttribution is used prior to addWorkgroupAttribution. Now I have revised the patch to re-introduce getNumPrivateAttribution we can leverage it here.

herhut added inline comments.May 20 2020, 9:40 AM
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
478

I don't understand this. Private attributions are always at the end. So using getBody().front().addArgument(...) would insert them at the end, where they belong. If you then use addWorkgroupAttribution, it will insert in front of the private one, as getNumWorkgroupAttributions would return 0, so it inserts directly after the function arguments.

whchung updated this revision to Diff 265295.May 20 2020, 10:19 AM

Simplify logic.

whchung marked 2 inline comments as done.May 20 2020, 10:21 AM
whchung added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
478

Yes you are right. I've revised the patch with simplified logic.

herhut accepted this revision.May 20 2020, 11:04 AM

Thanks!

This revision is now accepted and ready to land.May 20 2020, 11:04 AM
This revision was automatically updated to reflect the committed changes.
whchung marked an inline comment as done.
rriddle added inline comments.May 27 2020, 11:52 AM
mlir/include/mlir/Dialect/GPU/GPUOps.td
203

This doesn't make sense to me, when does GPUFuncOp ever have operands? Did you mean to use numArguments here? This looks like a lack of testing coverage if so.

whchung added inline comments.May 27 2020, 12:46 PM
mlir/include/mlir/Dialect/GPU/GPUOps.td
203

Thanks for catching this. Indeed this is a bug not covered by any existing tests. I'll submit another patch addressing this with a test.

whchung marked 3 inline comments as done.May 28 2020, 3:31 PM
whchung added inline comments.
mlir/include/mlir/Dialect/GPU/GPUOps.td
203

@rriddle I submitted D80766 to fix this.