This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] NFC: put SPIR-V attributes in separate files
ClosedPublic

Authored by antiagainst on Mar 9 2020, 1:58 PM.

Diff Detail

Event Timeline

antiagainst created this revision.Mar 9 2020, 1:58 PM
Herald added a project: Restricted Project. · View Herald Transcript
Harbormaster completed remote builds in B48599: Diff 249213.
Harbormaster completed remote builds in B48604: Diff 249218.

I am not sure its a good idea having so many headers. I am OK with having all of them in the same file. In this case specifically, the interface ABI and the target-env, vce attributes are needed for clients who are trying to serialize to SPIR-V binary. It makes sense to have them in one file

The diff has a whole lot of red, I am not sure where the methods were put. Is this an issue with the uploaded Diff?

mravishankar requested changes to this revision.Mar 11 2020, 12:07 PM
This revision now requires changes to proceed.Mar 11 2020, 12:07 PM

Address comments

The diff has a whole lot of red, I am not sure where the methods were put. Is this an issue with the uploaded Diff?

They are split into two files. If you check the two read parts, they compose the original file. It's showing as red from the new file's perspective.

I am not sure its a good idea having so many headers. I am OK with having all of them in the same file. In this case specifically, the interface ABI and the target-env, vce attributes are needed for clients who are trying to serialize to SPIR-V binary. It makes sense to have them in one file

Smaller headers offer better organization structure and faster compilation time. For users of target/ABI stuff, it does not affect things given that the new header is included anyway.

mravishankar accepted this revision.Mar 12 2020, 2:27 PM
This revision is now accepted and ready to land.Mar 12 2020, 2:27 PM
This revision was automatically updated to reflect the committed changes.