[mlir][acc] Introduce acc data bounds accessors #156545
Conversation
Add acc.get_lowerbound, acc.get_upperbound, acc.get_stride, and acc.get_extent operations to extract information from acc bounds. This simplifies the arguments needed for recipes when handling slices and makes bound information consistent with data clauses. Update recipe documentation to clarify argument ordering and add examples demonstrating slice handling with bounds arguments.
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
I think this makes a lot of sense, thanks!
| let description = [{ | ||
| This operation extracts the upperbound value from an `acc.bounds` value. | ||
| If the data bounds does not have an upperbound specified, this operation | ||
| uses the extent to compute it. |
There was a problem hiding this comment.
This description is not yet implemented :) I just captured the intent. Sorry if it misled.
There was a problem hiding this comment.
Ah, no, I assumed as much. Just awesome that it is the intent, saves a lot of other work.
| must yield the privatized copy. | ||
| 2. The destroy region specifies how to destruct the value when it reaches | ||
| its end of life. It takes the privatized value as argument. | ||
| its end of life. It takes the original value, the privatized value, and |
There was a problem hiding this comment.
Curious what the use of the privatized version is here? Which should I be destroying, the 1st one, right?
There was a problem hiding this comment.
The way I just documented this is that the original (read this as "non-private") variable is always passed in as the first argument for consistency. So it is the second one that should be destroyed. Are you OK with this?
There was a problem hiding this comment.
I don't see the first argument being 'useful' here? But I have no problem with it, I'll just have to 'note' it.
At one point I think the 'verifier' should start doing some level of failures for this signature though, which would help catch any issues I have.
There was a problem hiding this comment.
See here: #156716
I've changed the clang lowering to follow this signature.
There was a problem hiding this comment.
I'm not sure the first argument makes sense.
There was a problem hiding this comment.
I added it for consistency so all regions take it as first argument. In all other regions including init, copy, combiner it is taken as first argument.
Although I am not sure it is useful for CIR and FIR, I can imagine cases where it can be. One scenario is when a dialect's type system does not encode sizes and it needs to be recovered from IR so that destroy can properly deallocate. I am definitely grasping at straws here - so hopefully the consistency reason is enough to justify it.
There was a problem hiding this comment.
No strong opinion on that so I'll leave it to you.
There was a problem hiding this comment.
At one point I think the 'verifier' should start doing some level of failures for this signature though, which would help catch any issues I have.
I agree. I have not added verifier yet because Flang does not yet comply with this.
Patch llvm#156545 is introducing a different syntax for the 'destroy' section of a recipe, which takes the 'original' value as the first argument, and the one-to-be-destroyed as the 2nd. This patch corrects the lowering to match that signature.
Patch #156545 is introducing a different syntax for the 'destroy' section of a recipe, which takes the 'original' value as the first argument, and the one-to-be-destroyed as the 2nd. This patch corrects the lowering to match that signature.
3 participants
Add acc.get_lowerbound, acc.get_upperbound, acc.get_stride, and acc.get_extent operations to extract information from acc bounds.
This simplifies the arguments needed for recipes when handling slices and makes bound information consistent with data clauses. Update recipe documentation to clarify argument ordering and add examples demonstrating slice handling with bounds arguments.