This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Moving RVV intrinsic type related util to clang/Support
ClosedPublic

Authored by kito-cheng on Mar 18 2022, 1:00 AM.

Details

Summary

We add a new clang library called clangSupport for putting those utils which can be used in clang table-gen and other clang component.

We tried to put that into llvm/Support, but actually those stuffs only used in clang* and clang-tblgen, so I think that might be better to create clang/Support

Diff Detail

Event Timeline

kito-cheng created this revision.Mar 18 2022, 1:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2022, 1:00 AM
kito-cheng requested review of this revision.Mar 18 2022, 1:00 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 18 2022, 1:00 AM
khchen accepted this revision.Mar 21 2022, 6:21 PM

LGTM.

This revision is now accepted and ready to land.Mar 21 2022, 6:21 PM
This revision was landed with ongoing or failed builds.Mar 27 2022, 11:35 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a comment.EditedMar 28 2022, 1:07 AM

Seems that there is a circular dependency problem: 7c7e7770b7176f452d40f4e7ac545aaee1b19c4d (reverted).

I'd suggest that we revert this patch.

Having target specific stuff in LLVMSupport is always fishy.
Yes, we have ARMAttributeParser.h and ARMBuildAttributes.h for a long time, then RISCV* headers came along. They are not nice but benign since they introduce no circular dependency.
I can kinda understand the new header because clang-tblgen only depends on LLVMSupport, but this circular dependency does require more thoughts.

Hi @MaskRay, @akuegel has removed the llvm/TableGen/*, so I guess the issue you mentioned in https://github.com/llvm/llvm-project/commit/c0eb9b4cdef6049ebabb4018d3c9dcb0dc699868 is resolved by https://github.com/llvm/llvm-project/commit/268f24d2ea6ae92853c68c4614722d9e769a9408

Are you happy if I recommit that with @akuegel's fix again?

Yes, I believe the issue with the circular dependency was already fixed by my latest commit. Sorry for first committing the wrong fix (just removing the headers without checking that they were needed to pull in raw_ostream header transitively).
So if you apply my fix, you should be able to land this again (or just revert the revert).

Ah, sorry, I did not see that the end state of @akuegel's 3 commits fixed the circular dependency.

However, I think my other concern still applies: the layering seems weird that the RISCV stuff is placed in LLVMSupport.
Any chance it can be fixed?
You may consider a more appropriate library and possibly let clang-tblgen depend on it.

The original patch added 600 lines while the subject just said "Move". This may be fishy as well.

Having TableGen related void RVVIntrinsic::emitCodeGenSwitchBody(raw_ostream &OS) const { in LLVMSupport is also weird.

So I think this patch deserves more discussion and should not be simply reapplied.

Oh, apology, I guess I just miss the discussion, let me revert that again.

kito-cheng reopened this revision.Apr 8 2022, 3:27 AM
This revision is now accepted and ready to land.Apr 8 2022, 3:27 AM
kito-cheng updated this revision to Diff 421477.Apr 8 2022, 3:28 AM

Changes:

  • Move those stuffs into clang/Support instead of llvm/Support
  • Keep table gen staffs in clang/utils/TableGen/RISCVVEmitter.cpp
kito-cheng edited the summary of this revision. (Show Details)Apr 8 2022, 3:29 AM
kito-cheng added a reviewer: MaskRay.

@MaskRay I've move those stuffs into clang/Support which is introduce by this patch, and also keep those table gen stuffs in clang-tblgen, did you mind take a look?

Thanks :)

kito-cheng retitled this revision from [RISCV][NFC] Moving RVV intrinsic type related util to llvm/Support to [RISCV] Moving RVV intrinsic type related util to clang/Support.Apr 8 2022, 5:34 AM
MaskRay accepted this revision.Apr 8 2022, 4:29 PM
MaskRay added a subscriber: aaron.ballman.

llvm-tblgen only depends on LLVMSupport. As analogy, clang-tblgen depending on the new clangSupport makes sense to me.
Target-specific files in a *Support library is still a bit weird, but I cannot think of a better solution.

Hope other clang folks (e.g. @aaron.ballman) can confirm this is fine.

Errr, I'm not strongly opposed, but at the same time, this doesn't feel like a Support kind of thing. My understanding of llvm/Support is that it's primarily for generally useful utilities that are going to be used across the entire llvm project (or beyond). This is a very specific interface that's only interesting to TableGen as best I can tell. Why shouldn't this live in clang/utils/TableGen along with the others?

clang/include/clang/Support/RISCVVIntrinsicUtils.h
1 ↗(On Diff #421477)

Comment here seems off (this isn't a .cpp file, it's a .h)

Hi @aaron.ballman:

Why shouldn't this live in clang/utils/TableGen along with the others?

We plan to use those stuffs on clang side in https://reviews.llvm.org/D111617, my original change was put those stuffs on llvm/Support, but actually those stuffs are only used for clang and clang-tblgen, so that's why we try to create clang/Support.

It's target specific but need to used in clang and clang-tblgen so target specific stuffs should putting that in llvm/lib/Target/RISCV in theory, but that made clang dependent on that.

Thanks :)

Fix comment in RISCVVIntrinsicUtils.h

kito-cheng marked an inline comment as done.

Fix comment in RISCVVIntrinsicUtils.h...again :P

Hi @aaron.ballman:

Why shouldn't this live in clang/utils/TableGen along with the others?

We plan to use those stuffs on clang side in https://reviews.llvm.org/D111617, my original change was put those stuffs on llvm/Support, but actually those stuffs are only used for clang and clang-tblgen, so that's why we try to create clang/Support.

It's target specific but need to used in clang and clang-tblgen so target specific stuffs should putting that in llvm/lib/Target/RISCV in theory, but that made clang dependent on that.

Thank you for the explanation. I still don't think this is really "Support" material, but I'm also struggling to think of a better place to put it in an existing directory in Clang aside from Basic, but that would still be a bit of a layering violation it feels like. So I think I'm convinced that Support is a reasonable place to put it.

Should it live within a RISCV direction inside of the Support directory? Or should we use folders like that for host platform support files instead of target platform support files (as the LLVM Support directory appears to do)?

Thank you for the explanation. I still don't think this is really "Support" material, but I'm also struggling to think of a better place to put it in an existing directory in Clang aside from Basic, but that would still be a bit of a layering violation it feels like. So I think I'm convinced that Support is a reasonable place to put it.

Actually I tried to put that on clang/Basic before, and end up with the clang/Basic is depended on clang-tblgen, and that made clang-tblgen depend on clang/Basic...then CMake report error for circular dependency :P

Should it live within a RISCV direction inside of the Support directory? Or should we use folders like that for host platform support files instead of target platform support files (as the LLVM Support directory appears to do)?

I saw LLVM/Support are just target specific file on the folder instead of many target folder, I guess there should not be too much target specific files in clang/Support, and we could reorg the folder organization once it getting complicated? What do you think?

aaron.ballman accepted this revision.Apr 15 2022, 9:14 AM
aaron.ballman added a reviewer: rsmith.
aaron.ballman added a subscriber: rsmith.

Thank you for the explanation. I still don't think this is really "Support" material, but I'm also struggling to think of a better place to put it in an existing directory in Clang aside from Basic, but that would still be a bit of a layering violation it feels like. So I think I'm convinced that Support is a reasonable place to put it.

Actually I tried to put that on clang/Basic before, and end up with the clang/Basic is depended on clang-tblgen, and that made clang-tblgen depend on clang/Basic...then CMake report error for circular dependency :P

Yup, that's exactly the issue I was thinking we'd hit. :-)

Should it live within a RISCV direction inside of the Support directory? Or should we use folders like that for host platform support files instead of target platform support files (as the LLVM Support directory appears to do)?

I saw LLVM/Support are just target specific file on the folder instead of many target folder, I guess there should not be too much target specific files in clang/Support, and we could reorg the folder organization once it getting complicated? What do you think?

Yeah, I think that's likely reasonable.

Giving my LGTM but adding @rsmith in case he has any code owner concerns over it. If you don't hear any concerns by Tue of next week, I think you're fine to land.

This revision was landed with ongoing or failed builds.Apr 20 2022, 6:13 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 20 2022, 6:24 AM

Please add a clear comment to the new library's CMakeLists.txt file that explains when to put code there and when in clang/lib/Basic. (The patch description does say it, but the patch description is harder to find than a comment in the cmake file.)