This is an archive of the discontinued LLVM Phabricator instance.

[IR] Make nosync, nofree and willreturn default for intrinsics.
ClosedPublic

Authored by sstefan1 on Aug 15 2020, 10:28 AM.

Details

Summary

D70365 will allow us to make attributes default. This
is a follow up to actually make nosync, nofree and will return
default.

Diff Detail

Event Timeline

sstefan1 created this revision.Aug 15 2020, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2020, 10:28 AM
sstefan1 requested review of this revision.Aug 15 2020, 10:28 AM

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?
Like: DisableDefaultAttributes.
At least we should have a comment.

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?

sstefan1 added inline comments.Aug 15 2020, 4:06 PM
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?

jdoerfert added inline comments.Aug 16 2020, 9:35 AM
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.

sstefan1 updated this revision to Diff 285912.Aug 16 2020, 4:48 PM

Add DefaultIntrinsic class with default attributes disabled.

jdoerfert added inline comments.Aug 16 2020, 7:02 PM
llvm/include/llvm/IR/Intrinsics.td
391

These should be default intrinsics now?

sstefan1 updated this revision to Diff 285939.Aug 17 2020, 1:57 AM

update gc intrinsics to DefaultIntrinsic

llvm/include/llvm/IR/Intrinsics.td
391

Yes, forgot to update them.

Ping.

What should be the next step here? There was no response on the RFC update.

nikic added a subscriber: nikic.Aug 25 2020, 2:31 PM
arsenm added inline comments.Aug 27 2020, 6:39 AM
llvm/include/llvm/IR/Intrinsics.td
33

Probably should group all the default attributes together

simoll added a subscriber: simoll.Aug 28 2020, 5:23 AM
sstefan1 updated this revision to Diff 294992.Sep 29 2020, 7:56 AM

group default properties together

Its been more than a month now. Are there any additional concern about this?

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.

sstefan1 updated this revision to Diff 295089.Sep 29 2020, 12:43 PM

precommit (local) coro test changes with update_test_checks

something with the coro tests and "print" is wrong.

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.

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.

sstefan1 updated this revision to Diff 295263.Sep 30 2020, 6:38 AM

Opt-out of default attributes for llvm.coro.* intrinsics.

jdoerfert accepted this revision.Sep 30 2020, 7:03 AM

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.

This revision is now accepted and ready to land.Sep 30 2020, 7:03 AM
nikic added a comment.Sep 30 2020, 7:15 AM

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?

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 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?

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?

sstefan1 updated this revision to Diff 295507.Oct 1 2020, 2:48 AM

update clang tests

Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 2:48 AM
sstefan1 updated this revision to Diff 295509.Oct 1 2020, 2:59 AM

opt-out of few more intrinsics
add comment for DefaultIntrinsic class

uabelho added a subscriber: uabelho.Oct 2 2020, 5:32 AM

@nikic do you have a preference on any of the suggested names?

sstefan1 updated this revision to Diff 298348.Oct 15 2020, 4:09 AM

Switch to opt-in approach. Intrinsic class now has the same meaning as bofore.
To get default attributes we now use DefaultAttrsIntrinsic.

jdoerfert accepted this revision.Oct 15 2020, 10:32 AM

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.

sstefan1 closed this revision.Oct 20 2020, 3:24 AM

Commited in rGfbfb1c790982.

Forgot to add the revision link.