- User Since
- Nov 2 2017, 3:15 PM (189 w, 4 d)
Reformatted documentation change to fit better on the page.
Applied jhuber6's suggestions:
Fri, Jun 18
Apr 30 2021
Apr 29 2021
Apr 8 2021
Thanks for all that work on the other tests! Still LGTM.
Apr 5 2021
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.
Mar 29 2021
Mar 27 2021
Mar 26 2021
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?
Mar 25 2021
I haven't looked through the tests yet, but the implementation logic looks right to me. Thanks!
Rebased, and addressed reviewer comments.
Mar 24 2021
Mar 23 2021
Please add tests before committing this again.
Mar 18 2021
Mar 17 2021
Thanks for the review. I'm rebasing and retesting, and then I'll push.
LGTM if @jhenderson is happy.
Mar 15 2021
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?