Same question as @jdoerfert , and are we passing SOLVVEVV inside llvm-testsuite?

LGTM!

LGTM!

Updated the checks in the hip test.

Thanks, added a test.

Added context.

This is a work-in-progress. RCCL requires using hipcc for the linking step, however the CMake file would not allow me to override the CMAKE_LINKER as hipcc. The reason to use hipcc is to handle static libraries created by -fgpu-rdc.

A few small comments, otherwise LGTM on the HIP header side.

Looks OK to me, please address other review comments. Thanks.

Fixed to yaxunl's comments, using ~0U , and allowing major only component.

@palves was able to resolve his request following @aaron.ballman's proposal. Closing this patch.

In D102508#2759906, @yaxunl wrote:

In D102508#2759905, @ashi1 wrote:

Looks good, let me verify on HIP builder.

It depends on https://reviews.llvm.org/D102507

Looks good, let me verify on HIP builder.

In D100060#2727182, @tra wrote:

In D100060#2726888, @ashi1 wrote:

@tra have you seen this exception in your CUDA buildbots before?
https://lab.llvm.org/staging/#/builders/152/builds/3

Sorry, I haven't seen it before and Can't tell what exactly is the bot unhappy about.
It says unsupported operand type(s) for %: 'WithProperties' , but it's not clear why.

@tra have you seen this exception in your CUDA buildbots before?
https://lab.llvm.org/staging/#/builders/152/builds/3

Revised to @palves' comments.

Revised to Artem's comments.

Added documentation changes.

In D100404#2709916, @palves wrote:

In D100404#2708957, @ashi1 wrote:

Please review again, I have changed the patch description, and revised to the comments.

This new patch will accept #pragma clang attribute (...) without push/pop, and store the filter details into a global attribute group.

Thank you! I really like the "#pragma diagnostics" parallel, makes it even easier to justify.

I've tested the patch against the GDB testsuite, using the global #pragma clang attribute (...) form, and it worked against the couple testcases I tried.

(Running the whole testsuite will take a while, but I don't expect problems, if one test works, others should too.)

Noobie question -- I didn't see any change to the documentation in the patch. Is that a separate repo?

Revised to @gkistanova 's comments.

Please review again, I have changed the patch description, and revised to the comments.

Moving this script to annotated/ directory. The AnnotatedBuilder.py now support running bash scripts.

Hi @gkistanova, I've changed the buildbot to use a local relative path (in annotated/hip-build.sh dir).
I will keep it as a local uncommitted script during bring-up. D100666 was submitted, and script_interpreter="python" is disabled for this bash script.

I agree that the no_pop variant may not be what we want, so removing it.

Revised to Artem's comments, moved --jobs to extra_args when are unspecified.

In D100404#2701988, @erik.pilkington wrote:

In D100404#2686753, @palves wrote:

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

Hmm, the problem I see with that is that you have to know that nobody else push-ed a #pca scope that you're inadvertently adding to. I'd prefer having a syntax that always added to the "global scope" regardless of the #pca context, which is what no_pop is effectively doing. I think I'd also be okay with downgrading the EOF error into a warning (which you could suppress in the -include file), assuming that we still warned when the #pca comes from a macro declared in a system header.

Fix unrelated pre-merge test failures. Something changed upstream, unrelated to this patch.

Fixed formatting issues.

I may be moving this patch and combining with D99894 , since we plan to add this to the public repo.

If no additional opinions, I think this current patch with no_pop serves the purpose and can be committed, while we discuss if we want to add global scope. @palves, does that work for you?

In D99894#2693229, @gkistanova wrote:

No. Cuda builders got that in the code base as a temporary solution while they are working on their annotated script. Exactly the same arguments as yours. And I see that being confusing, and don't think it worth it after all.

To get it working in the production you need to add the support for shell scripts anyway. With that in place you could put your script in the right place and keep working on that while your builder is staged. It seems the right thing to do.

Please feel free to ask if you have questions or will need help with adding the shell script support.

Ping arphaman and aaron.ballman. Also adding @erik.pilkington, who introduced namespaces to clang attribute, for opinions.

@gkistanova , is it okay to submit this with a full system path temporarily? /buildbot/hip-build.sh? Or do we have to switch to a python script?

Address Artem's comments.

Closing this revision, I have a patch to add no_pop variant of #pragma clang attribute push.
https://reviews.llvm.org/D100404
Alternatively, we could look into making no pop the default.

In D98201#2674942, @palves wrote:

Allowing pragma push to be unmatched, maybe with an explicit option to enable it, would probably be less controversial than adding a new pragma that duplicates existing functionality.

To be clear, the pragma that I mentioned allows unmatching -- #pragma clang force_cuda_host_device begin -- is a preexisting Clang pragma, not the one proposed by this review. It's highly inconsistent for one pragma to error out when unmatched, while the other doesn't. Would you suggest that the pre-existing #pragma clang force_cuda_host_device begin should error out when unmatched?

No, what I'm saying is that we can allow #pragma clang attribute push to be unbalanced if the user requests it. Injecting it with -include is a reasonable use case, IMO and you've correctly pointed out that there's no easy way to add a matching pop.

#pragma clang attribute appears to be a better and more generic mechanism for tinkering with attributes and I would prefer to use it instead of adding more pragmas that do about the same thing.

In D100060#2675099, @tra wrote:

Looks reasonable overall.

Few drive-by comments below for the pitfalls you may eventually run into later.

Revised to Artem's comments, and added empty.hip.

In D99997#2672735, @tra wrote:

I've added @beanz as the reviewer who has more cmake know-how.

I'm glad to see HIP support being added to the testsuite. I don't know what your plans are for bringing up an LLVM build/test bot for HIP. If you want, I could probably run a compile-only HIP bot on one of the VMs that run CUDA bots.

This HIP builder is dependent on two other patches:

Please note, this patch is under-development, and I've added it here, as open uncommitted review to allow HIP builder to use external script.
https://reviews.llvm.org/D99894

LGTM, thank you.

Merged DiagnosticParseKinds as requested.

In D98278#2618936, @zero9178 wrote:

In D98278#2616932, @mstorsjo wrote:

In D98278#2616916, @zero9178 wrote:

Add GetErrcMessages.cmake, which contains a cmake function to automatically get the error messages of various posix error codes needed by lit by running a small C++ program.
Currently ENOENT, EISDIR, EINVAL and EACCES are supplied.
These error messages are then currently supplied to clang, llvm and lld as the errc_messages config parameter.

Regarding Cross compiling: the function uses try_run which when cross compiling may use the CMAKE_CROSSCOMPILING_EMULATOR to run the code.

How does it behave if such a thing isn't hooked up? Ideally it'd fall back silently and these parts of tests would just fail, but not block things overall.

It will fall back to using Python's strerror, potentially failing if pythons strerror would not return the same strings (only the case for MSVC I believe).

Thank you for the review, please see latest test updates adding tests for static/constexpr local var combinations.