- User Since
- Nov 2 2017, 3:15 PM (243 w, 19 h)
Mon, Jun 27
May 31 2022
May 30 2022
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.
May 29 2022
May 27 2022
Thanks for the quick review.
May 26 2022
May 24 2022
Thanks. Will try to push tomorrow.
Fixed a bug in the new tests.
Apr 19 2022
I've made a few requests for minor test and doc extensions, but otherwise this LGTM.
Apr 16 2022
LGTM based on your discussion so far with the language committee, but please give Ravi some time to comment.
Apr 12 2022
@asavonic: Thanks for continuing to work on this feature!
Apr 4 2022
@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.
Mar 24 2022
Mar 21 2022
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.
Mar 18 2022
I've added a few other subscribers that have participated in lit discussions in the past.
Mar 15 2022
Mar 14 2022
Mar 11 2022
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.
Dec 13 2021
Dec 10 2021
Dec 9 2021
Other than the question I added inline, LGTM.
Dec 8 2021
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.
Dec 3 2021
Dec 2 2021
Removed incorrect fix, but kept other changes, as discussed.
Nov 12 2021
Nov 11 2021
- 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.
Nov 10 2021
Sep 3 2021
LGTM. Thanks for the fix.
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.
Sep 1 2021
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?
Aug 31 2021
Aug 27 2021
I'll have to work on landing next week when I can watch the bots.
Thanks for reviewing.
Aug 20 2021
Aug 17 2021
Thanks for working on this. I agree there's a bug. However, this patch doesn't fix the bug. It provides a workaround.
Aug 13 2021
Thanks! I'll wait for the D106510 review before trying to land.
Updated patch summary to reflect that extensions are now enabled by default.
Aug 12 2021
Addressed @ABataev's suggestions:
--order is nice.