I think this looks fine with one request for change: We "decided" to use enum attributes not string attributes (for willreturn and others).

In D63372#1551988, @uenoku wrote:

In D63372#1547756, @jdoerfert wrote:

Could you run this with the LLVM test suite [0]?
Do you see a difference in statistics, compile time and/or runtime?

If you don't have a computer to do so, let me know.

[0] http://llvm.org/docs/lnt/quickstart.html

I ran LLVM test suite but there was no significant difference in both execution time and runtime.
I want to see stats (NumFnWillReturn and etc.) but I don't know how to gather stats. I ran with TEST_SUITE_COLLECT_STATS=ON but I couldn't merge them. Can you tell me?

In D63379#1550752, @sstefan1 wrote:

In D63379#1548872, @sstefan1 wrote:

Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?

I already tried it on some of the other tests and it worked fine. I am going to that and test it some more, and update tomorrow morning.

Sorry for the late update. I ran all tests with -disable-nounwind-inference -attributor -attributor-disable=false and they pass. Is that enough?

Just realized that the basic attribute test in test/Bitcode/attributes.ll is missing, see https://reviews.llvm.org/D49165#change-K8gwLFRSXwEe for an example.

@reames Was the last comment a "LGTM"? I need to start getting these patches of my plate :(

Sorry for my delayed review, I want to move this ahead now and commit it.

In D62393#1548388, @ABataev wrote:

In D62393#1543858, @jdoerfert wrote:

In D62393#1542731, @ABataev wrote:

In D62393#1542638, @jdoerfert wrote:

In D62393#1542513, @ABataev wrote:

In D62393#1542471, @jdoerfert wrote:

I want to investigate the racy accesses further and make sure it is not a miscompile inside LLVM.

This is not a problem inside LLVM. The problem appears after optimizations performed by the ptxas tool (when it compiles PTX to SASS) at O3 with the inlined runtime.

I extracted the test case (see below) but I was not seeing the ERROR. How did you run the test case to see a different value for Count?

You need to compile it with the inlined runtime at O2 or O3.

When I run
./bin/clang -fopenmp-targets=nvptx64-nvida-cuda -O3 -fopenmp --cuda-path=/soft/compilers/cuda/cuda-9.1.85 -Xopenmp-target -march=sm_70 -fopenmp=libomp test.c -o test.ll -emit-llvm -S
I get

https://gist.github.com/jdoerfert/4376a251d98171326d625f2fb67b5259

which shows the inlined and optimized libomptarget.

And you need the latest version of the libomptarget

My version is from today Jun 13 15:24:11 2019, git: 3bc6e2a7aa3853b06045c42e81af094647c48676

We have problems in Cuda 8, at least, for arch sm_35

I couldn't get that version to run properly so I asked someone who had a system set up.
Unfortunately, the test.c [1] did not trigger the problem. In test.c we run the new test part in spmd_parallel_regions.cpp 1000 times and check the result each time.
It was run with Cuda 8.0 for sm_35, sm_37, and sm_70.

Could you share more information on how the system has to look to trigger the problem?
Could you take a look at the test case we run and make sure it triggers the problem on your end?

[1] https://gist.github.com/jdoerfert/d2b18ca8bb5c3443cc1d26b23236866f

You need to apply the patch D62318 to reproduce the problem for sure.

Could you run this with the LLVM test suite [0]?
Do you see a difference in statistics, compile time and/or runtime?

Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?

Update all uses of isDereferenceablePointer

Update according to API suggestion

What is supposed to happen if the elements in the pointer vector have different underlying objects?
We need to document that in the comment and test it.

In D62393#1542731, @ABataev wrote:

In D62393#1542638, @jdoerfert wrote:

In D62393#1542513, @ABataev wrote:

In D62393#1542471, @jdoerfert wrote:

I want to investigate the racy accesses further and make sure it is not a miscompile inside LLVM.

This is not a problem inside LLVM. The problem appears after optimizations performed by the ptxas tool (when it compiles PTX to SASS) at O3 with the inlined runtime.

I extracted the test case (see below) but I was not seeing the ERROR. How did you run the test case to see a different value for Count?

You need to compile it with the inlined runtime at O2 or O3.

When I run
./bin/clang -fopenmp-targets=nvptx64-nvida-cuda -O3 -fopenmp --cuda-path=/soft/compilers/cuda/cuda-9.1.85 -Xopenmp-target -march=sm_70 -fopenmp=libomp test.c -o test.ll -emit-llvm -S
I get

https://gist.github.com/jdoerfert/4376a251d98171326d625f2fb67b5259

which shows the inlined and optimized libomptarget.

And you need the latest version of the libomptarget

My version is from today Jun 13 15:24:11 2019, git: 3bc6e2a7aa3853b06045c42e81af094647c48676

We have problems in Cuda 8, at least, for arch sm_35

In D63315#1542999, @nicholas wrote:

Could you include a test with this patch?

Thanks for the quick feedback!

In D62393#1542513, @ABataev wrote:

In D62393#1542471, @jdoerfert wrote:

I want to investigate the racy accesses further and make sure it is not a miscompile inside LLVM.

This is not a problem inside LLVM. The problem appears after optimizations performed by the ptxas tool (when it compiles PTX to SASS) at O3 with the inlined runtime.

I extracted the test case (see below) but I was not seeing the ERROR. How did you run the test case to see a different value for Count?

You need to compile it with the inlined runtime at O2 or O3.

Simplify the interface

In D62393#1542505, @ABataev wrote:

In D62393#1542465, @hfinkel wrote:

It makes me suspicious of that too. But no one here believes that anyone was trying to subvert the system and produce an inferior result - it is very likely that everyone had, and continues to have, the best of intentions.

Maybe, just maybe, before starting treat someone's activity "suspicious" better to start to try to understand something? To read something, to ask the questions in the proper manner, etc.?

I want to investigate the racy accesses further and make sure it is not a miscompile inside LLVM.
I extracted the test case (see below) but I was not seeing the ERROR. How did you run the test case to see a different value for Count?

Can you make the test use an enum version, thus nofree not "nofree".

DereferenceableOrNullBytesGlobally -> DereferenceableOrNullGloballyBytes

I give up on this for now. If there is interest in this we can revive it later.

Minor fixes

In D62313#1540967, @uenoku wrote:

In D62313#1540817, @jdoerfert wrote:

As mentioned in D62766, we should make nofree an enum attribute. See D62766 for some pointer on how to do so.

As I see it, D49165 is rebased.
Which is better to wait for D49165 or to make nofree enum attribute in this patch?

In D61652#1540888, @reames wrote:

Wow, we have a huge amount of boilerplate when adding an attribute.

Addressed comments: Getter/Setter rename + for loop

The getter/setter functions are called "DereferenceableBytesGlobally" not "DereferenceableGloballyBytes". I would like to get some feedback on this.

Add all the mechanics for dereferenceable_globally we have for dereferenceable

As mentioned in D62766, we should make nofree an enum attribute. See D62766 for some pointer on how to do so.

In D61652#1540722, @reames wrote:

Sorry for the late reply. Replying selectively to key points.

In D61652#1520287, @jdoerfert wrote:

The discussion with @sanjoy lead to the change from scope to globally, maybe that resolves this?

It does. The globally wording seems broad enough. You're missing a bunch of detail from the original spec which needs to be preserved, but the basic notion seems reasonable.

Please elaborate on the details I'm missing,

The main thing is your missing the wording about restrictions on where the attribute can be applied (i.e. pointer types). Reading the existing dereferenceable_or_null wording more careful, we have precedent for just assuming it carries over to other dereferenceable* flavours, so I'll drop this objection. I may separately post a patch to refine the wording, but this is not a key issue.

Now, my remaining concern. This patch, as structured, introduces a number of purely definitional miscompiles into LLVM. There are existing transformations which assume the dereferenceable global property for correctness. By introducing a new attribute with the old semantics, and not updating all of the existing code to use the new attribute (code, tests, etc..), you're are defining (via the langref change) existing optimizations to be incorrect.

I really don't think that's okay. Particular, since it should be very easy to fix. A simple find replace in source and tests which replaces checks for dereferenceable with checks for dereferenceable_globally should be all that's needed.

I'm specifically asking that this change not be committed - despite Hal's LGTM - before this point is addressed.

p.s. I wouldn't be thrilled by it, but I'd even be okay with a clear statement of intent to handle this in follow up changes. It is more of a theoretical problem than a practical one in the near term.

Add argument definitions

Remove the alignment part

We need more test cases, e.g. as the one below which shows some limits of the current impl.

@nicholas If you want to discuss any of this further, please say so.

Is this still an RFC or by now an accumulation of changes we actually want to make? I added to comments assuming the latter.

This looks good to me (LGTM). Give it a bit of time though for people familiar with this code to take a look if they want.

More comments. Can you diff it against the master branch so one can easily see all changes?

Generally fine, three smaller comments though.

This is a clarrification for some older comments.

In D63067#1538120, @lebedev.ri wrote:

In D63067#1538100, @sstefan1 wrote:

Why should the return values in nonnull.ll be noalias?

All functions returning pointer never actually capture it, or they return null. Isn't that enough?

This doesn't seem sufficient to me.
Consider:

file1.c
int* getter();
int* callee0() { // will get marked as noalias
  return getter();
}
int* callee1() { // will get marked as noalias
  return getter();
}
int entry() {
  int* p0 = callee0();
  int* p1 = callee1();
  // do p0 and p1 alias?
}

file2.c
int* global;
int* getter() { return global; } // oops

Update tests

Why should the return values in nonnull.ll be noalias?

In D62393#1533461, @ABataev wrote:

In D62393#1533454, @jdoerfert wrote:

> The standard has "levels" and "active-levels". Both need to be tracked but it seems we only have a single array?
>

They all are tracked on this array.

Where is that described?

Sorry, read the code. I'm not a debugger.

LGTM as well. Thanks!

In D59919#1535643, @nicholas wrote:

CHANGED: build-libcalls NumNoUnwind 4526 -> 3382 ( -25.276%)

Why did the number of nounwinds drop?

Fix Typo

Use worklist instead of recursion, add tests

In D63046#1535808, @nicholas wrote:

In D63046#1535804, @nicholas wrote:

The current algorithm can deduce only for very simple cases.

• A function doesn't have any loop.
• A function doesn't have any recursion.

What about functions that exit the program, throw an exception, or longjmp?

Why does this depend on D63046?

Move the isKnownWillReturn isAssumedWillReturn and the to string function to the base class. Then place the base class in the header. Otherwise, LGTM.

@pcc So I thought about this. I don't think printing all attributes always helps. Testing for a single "Function Attrs" line will be very brittle, the use of multiple check lines will make tests harder to read/write/maintain and the IR output is cluttered. What if we have an output like this one under a flag? It would have all the benefits this patch has and, as you seem to dislike it, it would be on an opt-in basis. Though, I'm still unsure under which circumstance this patch would be worse than the status quo.

In D63046#1535452, @uenoku wrote:

Can you clang format the code?

I always clang format before submitting a patch.
Is there something wrong?

LGTM

In D60047#1535525, @aykevl wrote:

Phabricator says:

This revision was not accepted when it landed

but I believe that's in error because it was not marked as accepted (while it was in practice).

Thanks for looking at this. I'll update the patch asap

Cleanup leftover arguments

Update to new Attributor design and more tests

I haven't had time to read through the explanation that is to become a comment yet.