I won't review this properly till next week. Generally looks good so you can work on the next patch if you want to.

LGTM, I mean it is black magic but that is to be expected. One comment below.

The commit title has a stray { instead of [

Phab is not involved in the merge process, it just is a normal git push on top of the llmv-project.
Do you have commit rights already? If not I can commit this for you in a few days.

LGTM with one small nit

This should fix PR887: https://bugs.llvm.org/show_bug.cgi?id=887
Please verify that and include the test case from there. We should mention it in the commit message and close the bug if it works.

LGTM with some minor things you should address before the merge.

What are instruction attributes? How would that look like? Is there a list discussion?

LGTM :)

In D72789#1822437, @jdoerfert wrote:

Can you comment on the test failures?

In D71989#1822326, @AndreyChurbanov wrote:

Having a copy function per type allows us to reuse it, otherwise we have one copy function per static task location (at worst). Either works for me I think.

I also would be OK with either option.
But note that using per-type functions will require more additions to the interface, like:

(..., num_objects, array_of_copy_wrappers, array_of_desctuctor_wrappers, array_of_obj_offsets).

Then the library can iterate over objects to copy-construct them, then iterate to destroy them after the task is complete. Without any possibility of inlining of any wrappers.

Per-task function only needs two additions - copy_wrapper and destructor_wrapper, all other details can live inside them, including possible inlining of constructors and destructors.

Can you comment on the test failures?

In D70412#1821136, @protze.joachim wrote:

In D70412#1820406, @Hahnfeld wrote:

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

In D71989#1820289, @rogfer01 wrote:

and we then call the copy constructors like this:

void *local_addr = ...;
for (kmp_uint32 u = 0; u < sizeof_copy_infos; ++u)
  local_addr = copy_wrapper_list[u](copied_obj_list[u], local_addr);

Just to confirm: that local_addr would be somehow linked to the task, I imagine it'd be initialized to something like task->shareds + offset_to_firstprivates, wouldn't it?

In D72475#1820962, @Tyker wrote:

i renamed the files to KnowledgeRetention.* to be more general. we can put other related utilities like the query API in the same files. i had a hard time finding a good name and i am not sure i succeeded what do you think ?

In D72666#1820862, @fhahn wrote:

In D72666#1818532, @jdoerfert wrote:

In D72666#1818407, @efriedma wrote:

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Yes we should probably just mark llvm.memset as IntrWriteMem.

LGTM.

I'm fine with this. If others are, please accept and don't wait for me.

LGTM.

LGTM.

In D71989#1819219, @AndreyChurbanov wrote:

the task create and task issue step are
conceptually not separated anymore as it is

I don't think this can work reliably. Because not all C++ objects can be mem-copied.
E.g. an object can keep its own address or reference, and mem-copy will make it broken.
This could be fixed by generating (optional) thunk routine which would create all needed objects
in the library-allocated space, and similar routine which would destroy all the created objects.

In D69930#1758037, @lebedev.ri wrote:

Also, this is missing some statistics, and maybe some debug counters.

In D72475#1817585, @Tyker wrote:

In D72475#1817423, @jdoerfert wrote:

`bool getAttributeFromAssume(Value &Base, CallInst &AssumeCI, Attribute::Kind Kind, int &Value)`

and similar for string attributes. Alternatively we could return a map from WhatOn values to attributes.

something like that seem fine.

I don't understand. What do you mean by "good names"? We have the operand bundle strings, right?

I meant that in the operand bundle the first argument is the WasOn and the second is optionally the attributes value.
but users shouldn't have to use magic numbers to access those fields.

In D72666#1818407, @efriedma wrote:

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

In D72475#1816055, @Tyker wrote:

In D72304#1815793, @fghanim wrote:

Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.

Is this something for future commits or do you want me to change these?
If the latter, how do I do that?

I see where you are going and we need this, however I just fiddled around with the entire isAssumedDead stuff to make the helper methods aware of dead uses (and instructions). I will see how we can combine this best with H2S and report here, OK?

@JonChesterfield Ping.

Only minor points are left. including:
Can we make string attributes and function scope attributes optional (under a command line flag)? Especially the uses created for the latter could easily confuse the call graph and similar passes. That said, we need to teach them to ignore llvm.assume uses (see below).

I added quite a bunch of inline comments again :( and some high level stuff below.

LGTM.

I'm generally fine with this. To avoid future problems we could make the RefCount private and introduce modifier methods that do the CONSIDERED_INF check always.

In D71617#1814061, @pgode wrote:

Thanks for reviewing further.

• Removed the Dependence flag.
• For the conditions to return 'true', in Pred: I observe that the correct condition should be below. This condition means that callsite_argument should not have nocapture and getCtxI() should be reachable from user. I think this, because for noalias propagation to happen w.r.t. (ii) condition, we should get true from checkForAllUses and Pred as well.
  
  ` // Doesn't have nocapture but reachable. if (!NoCaptureAA.isAssumedNoCapture() && ReachabilityAA.isAssumedReachable(UserI, getCtxI())) return true;
  
  `

Some high-level comments, haven't gone through it in detail

LGTM.

I think this is fine but I'll wait another day for input before I accept.

I found two minor problems that might solve the test issues we see.

Using post-dominator information is not safe. Take this code:

It helps if you use tags in the title of commits (and reviews), e.g., [Flang] .... Maybe we can make the commit message a bit more specific: "[Flang] Add Flang as subproject in cmake"

I'll push some fixes that should address the issues people saw (I hope) but there is at least one bug that the test suite exposed.

Sorry for my delay. I hope these are the last problems.

Update tests and address comment

Some initial comments. Can we split it in two patches (master/critical)?

Addressed comments

Rebase and better use of D72025

First add new edges before the SCC might get split up

More tests, late function removal and other improvements

LGTM.

In D71620#1802493, @uenoku wrote:

It seems the failure occurs when old pm is used.
I couldn't find what is causing this error.

@jdoerfert Could you take a look at this?

LGTM.

I can't test this but it looks reasonable, @protze.joachim what do you think?

In D71988#1799791, @Hahnfeld wrote:

In D71988#1799064, @jdoerfert wrote:

If this really brings a benefit, we need new entry functions and update the compiler(s).

We go to great lengths to align to cache lines in some structs, e.g., kmp_depnode, but we are literally wasting space and alignment for others. Even if we do not adopt this, we should start to adopt some best practices.

There's a difference though: kmp_depend_info_t is passed by the compiler and evaluated once by the runtime library. If I recall correctly, all information is stored in internal structures afterwards (the mentioned kmp_depnode being one of them). Thus I wonder if this change brings a measurable improvement when reading the data once is not on the critical path. Breaking the ABI on the benefit of saving a few bytes on the stack seems a bit overkill just for the sake of doing it.

In D69701#1799763, @arichardson wrote:

If I understand this correctly existing check lines between off/on directives should be copied verbatim. Could you add one (that does not match the auto-generated content) to the disabled part of the test?