Thanks!

Ping.

Below are some more details that seem worthwhile to capture somewhere
but that seem like too much for the review summary.

Thanks for the quick review.

Thanks for the quick review.

Thanks. Will try to push tomorrow.

Fixed a bug in the new tests.

I've made a few requests for minor test and doc extensions, but otherwise this LGTM.

In D122569#3458804, @asavonic wrote:

In D122569#3458476, @arichardson wrote:

While this is not a huge amount of code to be added to lit, I do wonder if we really need it. Wouldn't something like %feature{foo} that expands to 0 or 1 be sufficient? You could then use shell conditionals/test to avoid running lines.
Looking at the examples from D121727, you could do the following:

RUN: if [ %feature{ptxas} -ne 0 ]; then ptxas -c %t-nvptx.ptx -o /dev/null; fi
RUN: test %feature{ptxas} -ne 0 && ptxas -c %t-nvptx.ptx -o /dev/null

The variant with if is probably not going to work on windows.

LGTM based on your discussion so far with the language committee, but please give Ravi some time to comment.

@asavonic: Thanks for continuing to work on this feature!

@asavonic: Thanks for working on this. I've pointed out a few issues that need to be discussed before this lands. I also need to make another pass before I'm ready.

In D121647#3396314, @jdenny wrote:

The version of this patch that landed did not fix the openmp problem caused by D121276. I backed it out (locally) and applied the version that I originally tested on 3/15, and all seems fine (that is, check-all includes openmp tests). I'm testing at 3edec279dfa7.

The version of this patch that landed did not fix the openmp problem caused by D121276. I backed it out (locally) and applied the version that I originally tested on 3/15, and all seems fine (that is, check-all includes openmp tests). I'm testing at 3edec279dfa7.

In D121727#3393481, @tra wrote:

I believe that the whole "RUN" line must be handled atomically.

In D121727#3393453, @jhenderson wrote:

In D121727#3393386, @jdenny wrote:

In D121727#3393335, @jhenderson wrote:

Of course, this above has a lot of duplicated code, so it might be nice to make the if an if/else ternary-style. Spitball idea:

RUN: ... | FileCheck %s --check-prefix={%if windows {WINDOWS} else {NON-WINDOWS}}

I like this direction as it seems more flexible. Why do we need the outermost braces?

Because the else might be optional, so it clearly delineates the if/else block, as opposed to something using the term "else" immediately after the if part. I'm open to other suggestions on this though.

In D121727#3393335, @jhenderson wrote:

Of course, this above has a lot of duplicated code, so it might be nice to make the if an if/else ternary-style. Spitball idea:

RUN: ... | FileCheck %s --check-prefix={%if windows {WINDOWS} else {NON-WINDOWS}}

In D121727#3392944, @tra wrote:

when an existing construct (lit substitution) would work easily.

Could you elaborate? Do you mean the substitution of %ptxas with true if it's not available? Something else?

I've added a few other subscribers that have participated in lit discussions in the past.

In D121276#3386779, @phosek wrote:

I need LGTM on D121647, would one of you be willing to accept it?

In D121727#3390706, @mstorsjo wrote:

I think @jdenny is code owner (or the closest thing) for lit,

In D121276#3380870, @phosek wrote:

Can you test D121647 to see if it resolves the issue for you?

In D121276#3380266, @phosek wrote:

I tested this a bit more and it seems like changes to Tests.cmake aren't picked up by Ninja for some reason. It's not specific only to OpenMP, it seems to be affecting all other runtimes as well. If I manually rerun CMake, the tests are picked up but this should happen automatically.

IMHO, check-all should include openmp runtime tests, and this should be fixed. I'm not sure how openmp developers feel.

When building with -DLLVM_ENABLE_RUNTIMES=openmp, this patch causes openmp runtime tests to no longer be included by check-all.

In D60392#3188307, @jhenderson wrote:

In D60392#3184854, @thopre wrote:

In D60392#3184688, @jhenderson wrote:

Regarding the CHECK-ASSERT suggestion, do we need it? I was under the impression one of the earlier patches added a syntax like [[#VAR2, VAR1 + 3:]] or something to that effect, which achieves the same thing, right? Maybe I'm missing something though.

You mean [[#VAR2:VAR1+3]]? Why does that prevent the need for something like CHECK-ASSERT? You still can only refer to a variable defined in an earlier line.

Looking back at the example, I guess it doesn't, although you could use CHECK-SAME to achieve the same effect, I think. More generally, I'm not sure how much I like the idea of checks that don't actually match anything, but I could be persuaded, especially if we can leverage the same functionality to support the CHECK-DEFINE concept I proposed previously (but never got as far as finishing off).

In D60392#3182279, @thopre wrote:

Hi Joel,

Other than the question I added inline, LGTM.

Or maybe a CHECK-ASSERT would be clearer and more generally useful:

I understand the desire to be able to define and reference a variable within the same directive. However, as I understand it, this patch adds that ability in a way that introduces a new subtlety to FileCheck: the semantics of numeric variable references are then not always consistent. If you assume the wrong semantics, you can end up with a false pass of a test. I understand you've disabled the case of CHECK-NOT, but it does not appear to be the only case that can lead to false passes.

In D60392#3137020, @jhenderson wrote:

I've not looked at the testing in depth yet, and I'm a little concerned that I'm very cold on the surrounding code. I think it would be useful if someone else could take a look too (@jdenny maybe?).

Removed incorrect fix, but kept other changes, as discussed.

In D113824#3165416, @jdoerfert wrote:

I didn't see this patch before, sorry.

D114802 was the right fix.

• Addressed @jdoerfert's comments.
• Updated LLVM :: Transforms/OpenMP tests.

I just noticed that a few LLVM :: Transforms/OpenMP test need to be updated. I'll do that soon.

• Applied the same fix to the custom state machine, as suggested by @jdoerfert privately, and extended the new test to cover it. For that test on the NVIDIA Pascals I tried, fixing the custom state machine didn't appear to be needed. Perhaps in that version, the master thread manages to be selected for execution before other threads in its warp. However, fixing the custom state machine did prove important for that test on an AMD GPU I tried. Maybe another test would prove it's important for Pascals too, but I haven't looked for one.
• Moved fix to callers of generic state machine functions, as suggested by @tianshilei1992.
• Pointed out this fix is also relevant to AMD GPUs, as suggested by @JonChesterfield.

LGTM. Thanks for the fix.

In D109255#2983063, @jsji wrote:

In D109255#2983052, @ABataev wrote:

In D109255#2983017, @jdenny wrote:

This test is meant to check that the driver processes these options correctly. -cc1 isn't the driver and often ignores either the positive or negative version of an option.

Better not to do such checks, just need to check that driver correctly translates driver options to the frontend options (using -###), better to use -verify only with the frontend tests. Suggest to rework this test.

Make sense to me. How about we use _cc1 as Alexey suggested here (and rename the file to remove driver- prefix).

In D109255#2983018, @jsji wrote:

In D109255#2983017, @jdenny wrote:

This test is meant to check that the driver processes these options correctly. -cc1 isn't the driver and often ignores either the positive or negative version of an option.

OK, I can revert to driver + fsyntax-only if that is preferred?

This test is meant to check that the driver processes these options correctly. -cc1 isn't the driver and often ignores either the positive or negative version of an option.

Sounds good. I'll work on this today.

@grokos: Thanks for the reviews. Should I go ahead and land this patch series (D107925, D107926, D107927, D107928) after applying your suggestions, or do you think it would be better to wait for the discussion in D108569 to settle?

LGTM. Thanks.

I'll have to work on landing next week when I can watch the bots.

Thanks for reviewing.

Rebased. Ping.

Rebased.

Thanks for working on this. I agree there's a bug. However, this patch doesn't fix the bug. It provides a workaround.

Thanks! I'll wait for the D106510 review before trying to land.

Updated patch summary to reflect that extensions are now enabled by default.

Addressed @ABataev's suggestions:

--order is nice.