This is an archive of the discontinued LLVM Phabricator instance.

Add verifier for gpu.alloc op
ClosedPublic

Authored by akshaybaviskar on Jan 16 2022, 5:00 AM.

Details

Summary

Add verifier for gpu.alloc op to verify if the dimension operand counts and symbol operand counts are same as their memref counterparts.

Diff Detail

Event Timeline

akshaybaviskar created this revision.Jan 16 2022, 5:00 AM
akshaybaviskar requested review of this revision.Jan 16 2022, 5:00 AM
bondhugula accepted this revision.Jan 16 2022, 7:04 AM

LGTM - thanks. Commit summary is empty. (You can just copy-paste the commit title into the summary here.)

This revision is now accepted and ready to land.Jan 16 2022, 7:04 AM
mehdi_amini added a comment.EditedJan 16 2022, 1:05 PM

LGTM - thanks. Commit summary is empty. (You can just copy-paste the commit title into the summary here.)

In git that means just repeating twice the same line in the commit message, am I missing something?

I rather have a description of what the verified is implementing in the description.

bondhugula added a comment.EditedJan 16 2022, 7:44 PM

LGTM - thanks. Commit summary is empty. (You can just copy-paste the commit title into the summary here.)

In git that means just repeating twice the same line in the commit message, am I missing something?

I rather have a description of what the verified is implementing in the description.

Yes, that would be better, but in some cases, the title itself is a valid summary. Structurally, I think it isn't good to leave the summary empty (for whatever automation may be done in the future on git commits for example) -- the title and summary are distinct things. Duplicating the title into the summary itself is better than no summary I think even if it's adding zero information.

akshaybaviskar edited the summary of this revision. (Show Details)Jan 16 2022, 8:13 PM

I’ve never seen a title duplicated in a git commit message, do you have any reference recommending this?

akshaybaviskar edited the summary of this revision. (Show Details)Jan 16 2022, 8:21 PM

I’ve never seen a title duplicated in a git commit message, do you have any reference recommending this?

Hello Mehdi,

I have updated the commit summary, could you please check if it is okay?

Thanks,
Akshay

Yes, LG, thanks :)

Add verifier for gpu.alloc op to verify if the dimension operand counts and symbol operand counts are same as their memref counterparts.

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for the suggestion. Is it allowed to expose this function to other dialect?

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for the suggestion. Is it allowed to expose this function to other dialect?

Hello @herhut,

Could you please reply to this?

Thanks,
Akshay

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

It's not clear to me too where this should be exposed from. Also, we can't really reuse everything since memref.alloc/alloca are single-result ops while this one has two results. So things would still spill over unfortunately at both places.

Add verifier for gpu.alloc op

Add verifier for gpu.alloc op

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for the suggestion. Is it allowed to expose this function to other dialect?

Hello @herhut,

Could you please reply to this?

Thanks,
Akshay

This dropped off my review queue somehow. You are right, reuse seems difficult and might not be worth it after all.

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

The more common pattern is to make this a method AllocOp::verify and then have let hasVerifier = 1; in the td file.

This revision was automatically updated to reflect the committed changes.
bondhugula added inline comments.Feb 15 2022, 2:44 AM
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
1169

Looks like I landed this before I saw this comment. @akshaybaviskar could you please address this in a follow-up NFC revision?

ftynse added a subscriber: ftynse.Feb 15 2022, 2:56 AM

Note that while this was being iterated upon, the verifier field that this uses has been deprecated. Please send a follow-up updating it. https://discourse.llvm.org/t/psa-ods-op-verifier-field-is-being-removed/59714