D70365 will allow us to make attributes default. This
is a follow up to actually make nosync, nofree and will return
default.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Cool! the test effect shows how important this can be! I am unsure about the effect on the intrinsic definition but I guess it's not the worst. See below.
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
391 | Can we define a variable with value 1 to replace the trailing 1 here? We could also have a new IntrinsicClass that opts out, or one that opts in? class DefaultIntrinsic<list<LLVMType> ret_types, list<LLVMType> param_types = [], list<IntrinsicProperty> intr_properties = [], string name = "", list<SDNodeProperty> sd_properties = [] > : Intrinsic<ret_types, param_types, ..., /* disable_default_attributes */ 1> {} WDYT? |
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
391 | I had few other things in mind, but having a new Intrinsic class looks cleanest. Also it doesn't affect the parent patch. Do others have any opinions/objections? |
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
391 | No objections. And any solution that makes it clean and easy to opt-out is fine with me. |
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
391 | These should be default intrinsics now? |
update gc intrinsics to DefaultIntrinsic
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
391 | Yes, forgot to update them. |
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
31–32 | Probably should group all the default attributes together |
Adding @rjmccall as a reviewer as I'm unsure about some coroutine changes that appeared after rebase.
I've updated the coro tests with update_test_checks.py for simplicity. I think rest of the tests could be updated with the script as well, but I can always update them by hand if that is the preferred way.
We probably shouldn't be messing with the attributes on llvm.coro.* intrinsics. The rules for transforms around coroutine intrinsics aren't completely settled (see also https://reviews.llvm.org/D87817), but at least some of them can run arbitrary code.
I agree. Nothing was added to llvm.coro.* intrinsics. Changes to coro tests appeared after the rebase without touching coro intrinsics. I'm trying to find out what caused those changes. Any coroutine person willing to look into it as well would be appreciated.
Actually coro intrinsics had default attributes, completelly missed that. Sorry for the confusion.
I'll update the patch shortly.
I looked over all intrinsics again, 3 times I think we should opt-out. The tests look good. I'll send another reminder email because people need to update their target intrinsics as we move over.
LGTM with the nits below
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
351 | Add a comment here describing the difference to the class above. | |
972 | Opt out for these 2 | |
1158 | Opt out for these 3 | |
1229 | Make this Default too. |
I find the naming here confusing: Looking at this patch my expectation was that DefaultIntrinsic is used for intrinsics with default attributes, based on the name. Took me a while to understand it's actually the other way around. Shouldn't the Intrinsic <-> DefaultIntrinsic naming be inverted?
Possible, that will make it "opt-in" for existing intrinsics again. I'm fine with either. Alternative names for this scheme:
DefaultIntrinsic -> ExplicitAttrIntrinsic?
DefaultIntrinsic -> ConservativeIntrinsic?
I agree that it is not perfect. TBH that is one of the reasons for coro intrinsics confusion. I think there would always be some level of confusion between the two. Does OptOutIntrinsic sound better?
Switch to opt-in approach. Intrinsic class now has the same meaning as bofore.
To get default attributes we now use DefaultAttrsIntrinsic.
LGTM. We can change the name later if we need to.
This is now "opt-in" and (IIRC) we addressed all the known issues.
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
351 | Nit, = 0 Also, remove disable_default_attributes from the template args and pass 0 right away. No need to overwrite it by a user, they have Intrinsic for that. |
Probably should group all the default attributes together