- User Since
- Nov 2 2017, 3:15 PM (179 w, 4 d)
Thu, Apr 8
Thanks for all that work on the other tests! Still LGTM.
Mon, Apr 5
Ah, I see. Even though it wouldn't add very much code, it sounds like we agree that the conceptual complexity isn't worthwhile given the direction we'd like to go eventually. Thanks again for looking into it.
Mon, Mar 29
Sat, Mar 27
Fri, Mar 26
It seems the main issue left to address is tests that this patch breaks.
You say there are many test failures. Should I hold off reviewing the test changes already in this patch, or are those fine?
Thu, Mar 25
I haven't looked through the tests yet, but the implementation logic looks right to me. Thanks!
Rebased, and addressed reviewer comments.
Wed, Mar 24
Tue, Mar 23
Please add tests before committing this again.
Thu, Mar 18
Wed, Mar 17
Thanks for the review. I'm rebasing and retesting, and then I'll push.
LGTM if @jhenderson is happy.
Mon, Mar 15
Address @jhenderson's review.
Mar 11 2021
Mar 10 2021
Based on the comments in D97845, I see the semantic difference this patch addresses, but....
Other than a comment request, LGTM.
Rebased for contextual changes from D98086.
Applied @thopre's suggestions.
Mar 9 2021
I have no immediate plans to pursue this again, but maybe one day. Feel free to take over. Be sure to read the RFC thread. There was some delay waiting on another patch from someone else, but I've lost track.
Mar 7 2021
Rebased for changes to D98086.
I've generalized this patch to also handle errors that occur after matching. One issue is that PrintNoMatch was handling those, but I moved the handling to PrintMatch so that we actually report the full match that was found. That seems like useful information and helps with -dump-input.
Mar 6 2021
Actually, I do need to rethink the logic for that overflow case: PrintNoMatch is being called after a match, so its diagnostics don't make sense in that case. Sorry for the noise. I'll work on it.
This patch also extends input dumps to include similar error
Mar 5 2021
Mar 4 2021
As discussed, I'll try a different solution for this issue this patch addresses, probably in a different review.
Rebased and updated for D93341's effect on -dump-input.
Thanks for the quick review. Sorry for the delay in pushing.
Thanks for the reviews!
Mar 3 2021
- Added test for mandatory offload with omp_get_num_devices() == 0.
- Added comments pointing to OpenMP spec issue 2669.
I may be lost. Why is --ignore-case still needed in llvm/test/tools/yaml2obj/ELF/DWARF/debug-gnu-pubnames.yaml? I thought the special case for win32 was supposed to handle that.
Mar 2 2021
@thopre It's been almost a month. I suggest we move forward with this and address any complaints post-commit.
One more question: How do I test the new check? For cuda offload, I can set CUDA_VISIBLE_DEVICES to an empty string, but I haven't found a general way.
In anticipation of the spec change, I've updated the patch to check the omp_get_num_devices() == 0 case. A few questions:
Mar 1 2021
Thanks for pointing that out (OpenMP spec issue 2669). I wasn't aware it was changing. I'll wait until that settles.
In every case where this patch inserts a check, there is code immediately following it that calls HandleTargetOutcome(false, loc);, usually because CheckDeviceAndCtors(device_id) != OFFLOAD_SUCCESS. The inserted check prevents that from code from executing. Is that what you're asking?
While it's beyond what I need at the moment, I agree that test_phases looks better than early_tests, late_tests, etc.
Feb 28 2021
If a lit invocation runs multiple test suites that contain identically named test files, is there any way to make an --xfail entry apply to only one of them?
Feb 27 2021
Feb 26 2021
One property of this patch that has bothered me is that OMPCanonicalLoop is not a loop. Instead, it's an AST node that is sandwiched between a directive and a loop to contain extra information about the loop. The TreeTransform issues we've been discussing highlight how that extra node can be confusing.
Feb 25 2021
Other than the additional comments I've requested, LGTM. Thanks for the explanations and the changes.
Feb 24 2021
Feb 22 2021
Addressed @jhenderson's review. Rebased.