search

tensorflow / mlir

"Multi-Level Intermediate Representation" Compiler Infrastructure
1.73k stars 257 forks source link

Allow dialect to create friendly names for region arguments #329

Closed flaub closed 4 years ago

flaub commented 4 years ago

This supersedes PR #214

River707 commented 4 years ago

(Github won't let me respond to an outdated conversation, which is annoying).

It's not clear to me how to group block argument names What I had in mind for "grouping" is just how the interface with the 'setValueName' function works, not how the IR is actually printed. If we "grouped" block arguments, it would just mean that each argument should be prefixed with that name. Though, given that block arguments function a bit differently than operation results, it should be fine to have the behavior be slightly different. This difference should be clearly marked in a comment on the method though.

flaub commented 4 years ago

I don’t get why performance would be a consideration for the TestDialect.

flaub commented 4 years ago

(Github won't let me respond to an outdated conversation, which is annoying).

It's not clear to me how to group block argument names What I had in mind for "grouping" is just how the interface with the 'setValueName' function works, not how the IR is actually printed. If we "grouped" block arguments, it would just mean that each argument should be prefixed with that name. Though, given that block arguments function a bit differently than operation results, it should be fine to have the behavior be slightly different. This difference should be clearly marked in a comment on the method though.

I haven’t found a need for this feature. Could we do this as a follow up PR?

River707 commented 4 years ago

Coding standards apply across the entire codebase, the test dialect is no excuse to be inefficient/inconsistent.

flaub commented 4 years ago

Actually, all of this hooking is one way to achieve custom names. But really, all I want is a way for having custom names. Would it make more sense for MLIR to treat names as first class entities? What if a Value has an optional name? That’s the common piece between both this new hook and the existing op result hook. I don’t know where it would best be stored, but it at least makes sense for the textual representation.

joker-eph commented 4 years ago

Coding standards apply across the entire codebase, the test dialect is no excuse to be inefficient/inconsistent.

I wouldn't try to anchor too much on the "efficiency" aspect for testing-only code, but I am +1 on the "consistency" argument: it is just easier if the whole codebase is following the same idioms everywhere (and when I review code I don't necessarily realize I am reading some testing only code sometimes).

River707 commented 4 years ago

(Github won't let me respond to an outdated conversation, which is annoying).

It's not clear to me how to group block argument names What I had in mind for "grouping" is just how the interface with the 'setValueName' function works, not how the IR is actually printed. If we "grouped" block arguments, it would just mean that each argument should be prefixed with that name. Though, given that block arguments function a bit differently than operation results, it should be fine to have the behavior be slightly different. This difference should be clearly marked in a comment on the method though.

I haven’t found a need for this feature. Could we do this as a follow up PR?

SGTM