Hi everyone. Since this has been accepted for some time now, we're planning to commit this on Monday (December 16th). Please let us know if there is anything else we should address.

Hi everyone. Since this has been accepted for some time now, we're planning to commit this on Monday (December 16th). Please let us know if there is anything else we should address.

Hi everyone. Since this has been accepted for some time now, we're planning to commit this on Monday (December 16th). Please let us know if there is anything else we should address.

Hi everyone. Since this has been accepted for some time now, we're planning to commit this on Monday (December 16th). Please let us know if there is anything else we should address.

Hi everyone. Since this has been accepted for some time now, we're planning to commit this on Monday (December 16th). Please let us know if there is anything else we should address.

Hi everyone. Since this has been accepted for some time now, we're planning to commit this on Monday (December 16th). Please let us know if there is anything else we should address.

I like this reorganisation. LGTM!

Maybe we can have a test for this? Otherwise LGTM.

In D101701#2735040, @reames wrote:

I am neutral on the proposed change, but I would very much like to hear from @sstefan1, the original author of the nofree attribute.

In D100875#2705331, @xbolva00 wrote:

Looks ok as a fix. I am just wondering if some other places should be checked as well.

@lebedev.ri good to go?

update comment

I just want to mention that this is just a starting point, as suggested in D99769. It is definitely not our attention to rush and do this without an RFC or numbers that are in favour of the switch.

In D100339#2685448, @xbolva00 wrote:

In D100339#2685402, @sstefan1 wrote:

In D100339#2684938, @kuter wrote:

Interesting. Do we have near term plans for adding the Attributor to the default pass pipeline ?

That's the plan. We'll see how it goes.

Consider using @nikic's llvm compile time tracker for this patch and/or for full enablement of attributor.

http://llvm-compile-time-tracker.com/about.php

In D100339#2685909, @jdoerfert wrote:

Isn't the PassManagerBuilder part of this patch missing to actually enable the lightweight Attributor by default?

I thought we should flip the switch in a separate patch. We would only need to make -enable-attributor=all (or some other configuration) the default.

In D100339#2684938, @kuter wrote:

Interesting. Do we have near term plans for adding the Attributor to the default pass pipeline ?

Removing AAMemory*

In D99769#2676275, @fhahn wrote:

In D99769#2676226, @sstefan1 wrote:

I'm doing some testing with a lightweight attributor enabled by default. Can this wait few more days, until I get some results? If the numbers are good, maybe we won't need this after all?

That's great, but I think 'using a lightweight attributor pass by default' requires at least the following steps 1) add the pass with option to enable it, 2) have people test it, 3) collect data to make a convincing case for enabling it, 4) flip the default and deal with the fallout. I am not sure this can be done in a few days.

I'd recommend to start with sharing a patch that adds this lightweight attributor pass, so we can start with getting consensus what the scope of this pass should be and go from there (without tying ourselves to any particular timeframe)

I'm doing some testing with a lightweight attributor enabled by default. Can this wait few more days, until I get some results? If the numbers are good, maybe we won't need this after all?

LGTM

I guess the script should be committed first.

LGTM

In D94687#2498336, @jdoerfert wrote:

Some notable exceptions I think are exclusive loads and stores as well as the memory barrier intrinsics, for which nosync does not apply I think.

FWIW, that sounds about right.

Really happy people start to use this for target intrinsics as well.

In D92404#2436313, @mtrofin wrote:

Gentle reminder - @jdoerfert , is blank-allowing all the Transforms/Attributor to have unused prefixes "as intended"? (I remember you indicating that was the case)

Definitely not intentional, sorry.

Cool :)

In D90103#2410284, @abidmalikwaterloo wrote:

The tests should be in llvm-project/llvm/test/Transforms/OpenMP.

On the first look, you should at least clang-format this as it is not very readable right now.

In D89826#2348164, @nhaehnle wrote:

Oh, and on the change itself: I'm not familiar enough with the attributor framework to judge the implementation, but the described reasoning (being able to make deductions from indirect calls) is sound.

Commited in rGfbfb1c790982.

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

In D75384#2317444, @abidmalikwaterloo wrote:

I have been trying to update my patch by following the steps on:

https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

However, I always end up loading nothing.

Only move the declaration

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

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

update clang tests

In D86021#2303405, @nikic wrote:

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?

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

Actually coro intrinsics had default attributes, completelly missed that. Sorry for the confusion.
I'll update the patch shortly.

In D86021#2301868, @efriedma wrote:

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.

precommit (local) coro test changes with update_test_checks

Adding @rjmccall as a reviewer as I'm unsure about some coroutine changes that appeared after rebase.

Rebase

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

group default properties together