In D29660#839736, @alekseyshl wrote:

In D29660#839728, @alekseyshl wrote:

Bad news, the bot is still red: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/7114

Disabled openmp-offload.c on Linux again: https://reviews.llvm.org/rL310772

In D34701#823166, @schandra wrote:

Hi Jonas

Did you have further comments on this commit?

Thanks
Sunita

Given the lack of additional comments, LGTM. Please adapt the commit description to reflect the changes during review.

I've committed the patch without an explicit answer from the IBM guys to unblock the release

Disable building by default.

In D34876#806869, @kcc wrote:

This check is a sanity check, please keep it, but feel free to change the constant to e.g. 0x100000000 (16x increase)

In D34888#799576, @gtbercea wrote:

Does this also include the fixes in the following revision?

https://reviews.llvm.org/D29905

In D34158#797170, @mibintc wrote:

The other test that fails is my own new test! It fails because I don't know how to set it up so the test thinks it has a gcc toolchain with version > 4.8. I tried using gcc-toolchain set to various other Linux toolchains that i see in the test/Driver/Inputs - none of them cause the gcc version to be in the range. I also tried using -ccc-installation=Inputs/ which I see being used for gcc version parsing. How can I set up the test so that the GCCInstallation has a Version >= 4.8? I test the new functionality from the console on Linux and can confirm it's working.

In D34876#796951, @alekseyshl wrote:

What's the stack size on CentOS? What was the point of the original check?

In D34701#794365, @sergiop wrote:

Thank you for the comments. I generated again the patch following your comments.

I don't see any new files...? I'm also not entirely sure that we should have Fortran tests? @jlpeyton

Some comments inline. In general you should consider posting an RFC on cfe-dev because this change will basically affect all compilations on GNU/Linux if the file is present.
Adding Richard (general maintainer) and Renato (ARM Linux) so they are aware.

Because I've seen this on other revisions as well: Please always subscribe the relevant mailing list!

In D33227#769276, @sdmitriev wrote:

I can change the name for this environment variable it as you suggested, but it will make it consistent with other environment variable's name that is already used by libomptarget - OMP_TARGET_OFFLOAD (look at openmp/libomptarget/src/omptarget.cpp, line 285).

Please always add the relevant -commits mailing list

LGTM. Have you run clang-format on omptargetplugin.h? Some lines look weirdly wrapped...

In D32734#748977, @beanz wrote:

What is the problem you're trying to solve here? This seems odd to me.

One minor drive-by comment. I think there is still one outstanding from Justin...

LGTM

LGTM

Does this change result in a lower runtime? Last time I tested clang-ykt on Pascal GPUs, 1024 threads were really the best thing to do...

LGTM, not currently used in Clang.

LGTM with one small note

LGTM unless Alexey still has objections

Ping

Is there a way to actually test the changed code? The current test changes seem unrelated...

Ping!

Ping! This gets pretty annoying...

Catch one more occurence.

Please format all comments as full sentences.

LGTM

Out of interest: This is a limition of the implementation, right? Because the standard only says: A throw executed inside a target region must cause execution to resume within the same target region, and the same thread that threw the exception must catch it.

Just noticed that this isn't specific to OpenMP. Could you move the test to Driver/cuda-external-tools.cu where the rest of ptxas is tested?

LGTM after fixing the check-prefix

LGTM. Please run clang-format before committing!

Having something with dashes behind other used prefixes (CHK-PTXAS, CHK-PTXAS-C, CHK-PTXAS-C-RELO) might not be optimal and break when other suffixes like -NOT or -SAME are added to lit. Please see inline about my suggestions but feel free to use others...

LGTM

Rebase and ping!

Rebase and ping.

LGTM

LGTM

LGTM

In D30739#706304, @fpetrogalli wrote:

AFAIK, none of the machinery required in LLVM to expose the mangled names in the vectorizer is present in trunk. There is a patch for x86 under review that does that: https://reviews.llvm.org/D22792

In principal looks good to me although I'm not really familiar with this part. Does that work for you if you have the declare simd in a header file and the implementation in another file? On x86_64 I currently get:

remark: loop not vectorized: call instruction cannot be vectorized

I thought the library would just compile fine and that the CUDA plugin should work without ELF support - turns out both don't because the CUDA plugin uses ELF to verify the binary header. This could certainly be solved differently but that's future work if someone is interested.

Ping

Another ping

Ideally, we should check if the linker supports --version-script, but currently this yields no benefit.

In D30733#697108, @jroelofs wrote:

As I said on D30214, it is inappropriate to be installing libc++ in the resource directory... please do not do that.

No build system will ever set -frtlib-add-rpath to enable this "feature". I'm for keeping this opt-out until we have configuration files to set this by default. Making it opt-in would weaken its main reason of existence: Not to break simple binaries for the user, and we can just drop it.

Rebase for recent refactoring and ping.

Rebase and adapt test added meanwhile.

In D30470#694814, @jlpeyton wrote:

Yes, OpenMP would be under the runtimes/ directory.

http://lists.llvm.org/pipermail/cfe-dev/2017-January/052512.html suggested to install runtime libraries (like libomp, but I think libc++ in certain cases may fall under the same category) to ResourceDir and I wanted the headers to be co-located. However, I can live with installing them to normal include

Adapt and add tests.

Reword comment