[mlir][spirv] Replace hardcoded attribute strings with op methods

[antiagainst]

Due to historical reasons we have quite some hardcoded strings for attribute names used in parser/printer and serializer/deserializer. Now in ODS we actually generate op methods for getting the names of attributes. It's better to replace the hardcoded strings for consistency. Specicially:

• Replace parser/printer attribute strings here. For example, branch_weights can be replaced with getBranchWeightsAttrName().strref().
• Check and replace such patterns in serializer too, e.g., here. Need to check all the files in serializer library. But don't need to change the autogenerated ones given those are consistent for sure due to autogen.
• Do the same for deserializer.

[llvmbot]

@llvm/issue-subscribers-mlir-spirv

Author: Lei Zhang (antiagainst)

Due to historical reasons we have quite some hardcoded strings for attribute names used in parser/printer and serializer/deserializer. Now in ODS we actually generate op methods for getting the names of attributes. It's better to replace the hardcoded strings for consistency. Specicially:

• Replace parser/printer attribute strings here. For example, branch_weights can be replaced with getBranchWeightsAttrName().strref().
• Check and replace such patterns in serializer too, e.g., here. Need to check all the files in serializer library. But don't need to change the autogenerated ones given those are consistent for sure due to autogen.
• Do the same for deserializer.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. In the comments of the issue, request for it to be assigned to you.
2. Fix the issue locally.
3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a Git commit.
5. Run git clang-format HEAD~1 to format your changes.
6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Lei Zhang (antiagainst)

Due to historical reasons we have quite some hardcoded strings for attribute names used in parser/printer and serializer/deserializer. Now in ODS we actually generate op methods for getting the names of attributes. It's better to replace the hardcoded strings for consistency. Specicially:

• Replace parser/printer attribute strings here. For example, branch_weights can be replaced with getBranchWeightsAttrName().strref().
• Check and replace such patterns in serializer too, e.g., here. Need to check all the files in serializer library. But don't need to change the autogenerated ones given those are consistent for sure due to autogen.
• Do the same for deserializer.

[haanhvu]

I'll work on this

[anishrajeev]

I am new to LLVM and am just trying to familiarize myself with the codebase, I started taking a look at this issue to dip my feet in the water and was wondering where I could read more about ODS generating op methods, specifically where I could access those generated getter methods for the attribute names? Thanks!

[antiagainst]

I am new to LLVM and am just trying to familiarize myself with the codebase, I started taking a look at this issue to dip my feet in the water and was wondering where I could read more about ODS generating op methods, specifically where I could access those generated getter methods for the attribute names? Thanks!

The TableGen backend for op definition that takes in .td files to generate .inc files is in this directory. It might be hard to read directly there given the string concatenation. Typically the .inc files are generated to a path matching the source .td files. For example, you can find .inc files for SPIR-V dialect in <build-dir>/tools/mlir/include/mlir/Dialect/SPIRV/IR.

[NNhanptnk]

I am also new to LLVM and would like to dive into this issue as well. Thank you for the information.

[SahilPatidar]

Hello @antiagainst,

I'm considering a series of replacements throughout the serialization codebase, such as changing op->getAttr("memory_access") to op.getMemoryAccessAttr() in serialization.

Are there any specific considerations or modifications you recommend for these replacements?
Your feedback is appreciated.

[SahilPatidar]

@antiagainst,
Should I make the replacement as mentioned above, or should I simply change the attribute name in the case of "memory_access" to op.getMemoryAccessAttrName()?

[SahilPatidar]

@antiagainst @kuhar,
Please let me know if there is anything wrong in my commit

[antiagainst]

@antiagainst @kuhar, Please let me know if there is anything wrong in my commit

Hi sorry for the late reply. What you have in the above commit looks good to me!

[tw-ilson]

Hi @antiagainst,
I have made some changes throughout the parsing code as you directed:
((tw-ilson@c794f24#diff-8d05b7e17af37e334c7676ca9e1affd75fcf10351ce85d201027aa298bf04918))

note that the following attribute names are not generated via ODS:

inline constexpr char kClusterSize[] = "cluster_size"; // no ODS generation
inline constexpr char kControl[] = "control"; // no ODS generation
inline constexpr char kFnNameAttrName[] = "fn"; // no ODS generation
inline constexpr char kSpecIdAttrName[] = "spec_id"; // no ODS generation

Also, I remove this assertion from the Op definitions in order to use the static methods in one place (parseSourceMemoryAccessAttributes), which might be objectionable:
// assert(name.getStringRef() == getOperationName() && "invalid operation name");

Let me know what you think
Tom Wilson

[antiagainst]

note that the following attribute names are not generated via ODS

Yeah some attributes not generated by ODS is fine.

Also, I remove this assertion from the Op definitions in order to use the static methods in one place (parseSourceMemoryAccessAttributes), which might be objectionable:
// assert(name.getStringRef() == getOperationName() && "invalid operation name");

This shouldn't be necessary? should avoid touching this part and revert the changes in parseSourceMemoryAccessAttributes.

[SahilPatidar]

@antiagainst,
I have completed some work on the Serializer and Deserializer codebase. Could you please review it?

Thank you,

[antiagainst]

Reopen given this is not done entirely.

[tw-ilson]

@antiagainst
I replaced all instances of the hardcoded attr names in the parser, and I was able to add back the assertion that I had previously removed.

[antiagainst]

@antiagainst I replaced all instances of the hardcoded attr names in the parser, and I was able to add back the assertion that I had previously removed.

Nice, thanks for the contribution, @tw-ilson!