This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Rename `OpAsmParser::OperandType` to `OpAsmParser::UnresolvedOperand`
ClosedPublic

Authored by zero9178 on Mar 21 2022, 8:06 AM.

Details

Summary

I am not sure about the meaning of Type in the name (was it meant be interpreted as Kind?), and given the importance and meaning of Type in the context of MLIR, its probably better to rename it. Given the comment in the source code, the suggestion in the GitHub issue and the final discussions in the review, this patch renames the OperandType to UnresolvedOperand.

Fixes https://github.com/llvm/llvm-project/issues/54446

Diff Detail

Event Timeline

zero9178 created this revision.Mar 21 2022, 8:06 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
zero9178 requested review of this revision.Mar 21 2022, 8:06 AM
zero9178 edited the summary of this revision. (Show Details)Mar 21 2022, 8:07 AM
zero9178 updated this revision to Diff 416950.Mar 21 2022, 8:15 AM

Rename occurrences in variable names as well as comments as well (Only found a few in Parser.cpp)

lattner added a comment.EditedMar 21 2022, 9:03 AM

Thank you for tackling this! I agree that "OperandType" isn't the right thing, but I don't think "OperandReference" is either. The key thing about this is that it models an occurrence of an SSA value in an operand position, but without having a type applied to it. "Operand" and "Reference" are somewhat redundant.

What do you think about a name like "UnboundOperand" or "OperandWithoutAType" (joking but may you have a better idea) or OperandSyntax or LexicalOperand?

Thank you for tackling this! I agree that "OperandType" isn't the right thing, but I don't think "OperandReference" is either. The key thing about this is that it models an occurrence of an SSA value in an operand position, but without having a type applied to it. "Operand" and "Reference" are somewhat redundant.

What do you think about a name like "UnboundOperand" or "OperandWithoutAType" (joking but may you have a better idea) or OperandSyntax or LexicalOperand?

Hmm. One of the names I had in mind as well was "OperandOccurrence". From the names you listed I like "UnboundOperand" the most (bit of a symmetry with resolveOperands as well).
I don't have a very strong opinion of any of those names however (except maybe "OperandWithoutAType" :) ).

Thank you for tackling this! I agree that "OperandType" isn't the right thing, but I don't think "OperandReference" is either. The key thing about this is that it models an occurrence of an SSA value in an operand position, but without having a type applied to it. "Operand" and "Reference" are somewhat redundant.

What do you think about a name like "UnboundOperand" or "OperandWithoutAType" (joking but may you have a better idea) or OperandSyntax or LexicalOperand?

Hmm. One of the names I had in mind as well was "OperandOccurrence". From the names you listed I like "UnboundOperand" the most (bit of a symmetry with resolveOperands as well).
I don't have a very strong opinion of any of those names however (except maybe "OperandWithoutAType" :) ).

I was leaning towards OperandOccurrence as well. But now that you point out the relationship with resolveOperands, I think that UnboundOperand or UnresolvedOperand would be good options too.

Thanks for tackling this!

Yeah, +1 for UnresolvedOperand; good point.

zero9178 updated this revision to Diff 417040.Mar 21 2022, 11:58 AM
zero9178 retitled this revision from [mlir] Rename `OpAsmParser::OperandType` to `OpAsmParser::OperandReference` to [mlir] Rename `OpAsmParser::OperandType` to `OpAsmParser::UnresolvedOperand`.
zero9178 edited the summary of this revision. (Show Details)

Rename to "UnresolvedOperand" instead of "OperandReferences" & clang-format

lattner accepted this revision.Mar 21 2022, 12:57 PM

Thank your for tackling this!

This revision is now accepted and ready to land.Mar 21 2022, 12:57 PM
zero9178 updated this revision to Diff 417071.Mar 21 2022, 1:24 PM

clang-format correctly this time & trigger build bot

This revision was landed with ongoing or failed builds.Mar 21 2022, 1:42 PM
This revision was automatically updated to reflect the committed changes.

Great work, thanks!