-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] Update checks in unsafe atomics test for AMDGPU #16342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
FIXES: #15377 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the ! might help make it clear we're checking for metadata.
Alternatively, we could maybe just check the whole line, without skipping anything ({{.*}})? I take it we can't use update_cc_test_checks.py to autogenerate everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the !, seems like update tests script is having a hard time trying to parse the output of the tests:
!93 = !{!13, !32}
Traceback (most recent call last):
File "./llvm/utils/update_cc_test_checks.py", line 561, in <module>
sys.exit(main())
File "./llvm/utils/update_cc_test_checks.py", line 392, in main
for k, v in get_line2func_list(ti.args, clang_args).items():
File "./llvm/utils/update_cc_test_checks.py", line 127, in get_line2func_list
ast = json.loads(stdout)
File "/usr/lib64/python3.6/json/__init__.py", line 354, in loads
return _default_decoder.decode(s)
File "/usr/lib64/python3.6/json/decoder.py", line 342, in decode
raise JSONDecodeError("Extra data", s, end)
json.decoder.JSONDecodeError: Extra data: line 23586733 column 2 (char 1320540514)
In any case not sure if it would bring that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aw, fair enough. Just that automating the tests is like clang-format (don't have to think about it) and means we'd catch any unwanted change to the output, like if the alignment was wrong, or an update brought in extra metadata that we don't expect.
We should probably look at fixing that script for SYCL (if that is indeed the problem) - I think it would help the upstreaming effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it to the internal issue tracker.
b0ad3de to
fb67144
Compare
|
CUDA failure is not related, but will re-run the tests. |
fb67144 to
7771d32
Compare
|
@ldrumm if you're happy with the patch, it should be ready to roll. |
The need for updated check was caused by a move from using AMD builtins in favour of IR instructions with correct metadata attached, see: b5e63cc
I travelled back in time to:
and verified that generated assembly (cpp -> IR -> s) is the same for before and after the builtin change.