[FIRRTL] AnnoTarget: use LLVM style casts #7002
Conversation
This changes AnnoTargets to use LLVM style casts instead of "trailing" member functions. The LLVM style casts are now usable with AnnoTargets with improvements to the upstream framework. The current style of cast used with AnnoTargets will no longer be supported by TypeSwitch, so this PR switches to the recommended idiom.
| struct CastInfo< | ||
| To, From, | ||
| std::enable_if_t< | ||
| std::is_same_v<circt::firrtl::AnnoTarget, std::remove_const_t<From>> || |
There was a problem hiding this comment.
What does the is_same_v part of this do that isn't covered by is_base_of_v? Maybe something with incomplete types?
Looking into this a bit, is_base_of here returns false if the types are incomplete (apparently it's undefined behavior for second type to be incomplete) (so really should avoid making it possible to use this with an incomplete derived type, which is easy by just putting it at the end like you've done).
Anyway, is the is_same_v doing anything? (I don't think so?)
I see this combination in some MLIR code but nowhere else, hmm. Not blocking, just curious.
https://github.com/llvm/llvm-project/blob/50b45b24220ead33cf5cedc49c13e0336297e4eb/llvm/include/llvm/Support/Casting.h#L275-L280 for example seems just fine using only std::is_base_of_v.
And I was wondering why we need to do the special handling for self/upcasts ourselves, but probably by defining this we override that behavior?
FWIW this is defining cast support /from/ AnnoTarget to anything else. This appears to be done in some MLIR bits too. Might be good idea to limit this for To that are also AnnoTarget-or-derived, so that (in theory) casting for other types could be specialized/defined (perhaps with different doCast than feeding the impl pointer to the constructor of To)?
There was a problem hiding this comment.
Anyway, is the is_same_v doing anything?
AFAICT it isn't doing anything, it might just be a mistake upstream because is_base_of is a little unintuitive. Quote from cppreference "Although no class is its own base, std::is_base_of<T, T>::value is true because the intent of the trait is to model the "is-a" relationship, and T is a T. "
FWIW this is defining cast support /from/ AnnoTarget to anything else. This appears to be done in some MLIR bits too. Might be good idea to limit this for To that are also AnnoTarget-or-derived, so that (in theory) casting for other types could be specialized/defined (perhaps with different doCast than feeding the impl pointer to the constructor of To)?
I tried to limit the scope of this specialization to only apply when To is a subclass of AnnoTarget, but since is_base_of does not support incomplete types, I first need to check that the To type is complete. Without this check, this specialization is considered for all sorts of random things in LLVM and causes an explosion of errors. It seems like it might be possible to create a template which checks this, but seems like a bad idea.
There was a problem hiding this comment.
I tried to limit the scope of this specialization to only apply when
Tois a subclass ofAnnoTarget, but sinceis_base_ofdoes not support incomplete types
Ahhh, right. Okay.
Weird because From could be incomplete too? But okay, and thanks for the links/summary/explanation!
|
Any chance we could merge this so that I can finish up the LLVM bump? |
Drive by comment: one way to handle this is to create a "stacked" PR. Unfortunately, GitHub doesn't have great support for this. However, you can fake it by rebasing the LLVM bump on top of this branch and then changing the base branch of the LLVM bump PR to point at this branch. When this PR lands, GitHub will automatically change the base branch of the LLVM bump PR to point at There are a number of edge cases that can cause this to break, but it kind of works and I tend to do it frequently. 🤓 And 👍 to landing this! |
|
Caught a whole bunch of deprecated casts all over CIRCT. Also TypeSwitch doesn't support null arguments anymore, so that had to be handled as well, it's all in the LLVM Bump PR |
This changes AnnoTargets to use LLVM style casts instead of "trailing" member functions. The LLVM style casts are now usable with AnnoTargets with improvements to the upstream framework. The current style of cast used with AnnoTargets will no longer be supported by TypeSwitch, so this PR switches to the recommended idiom.