Page MenuHomePhabricator

[openmp] Accept directory for libomptarget-bc-path
ClosedPublic

Authored by JonChesterfield on Sep 1 2021, 5:55 AM.

Details

Summary

The commandline flag to specify a particular openmp devicertl library
currently errors like:

fatal error: cannot open file
      './runtimes/runtimes-bins/openmp/libomptarget':
      Is a directory

CommonArgs successfully appends the directory to the commandline args then
mlink-builtin-bitcode rejects it.

This patch is a point fix to that. If --libomptarget-amdgcn-bc-path=directory
then append the expected name for the current architecture and go on as before.
This is useful for test runners that don't hardcode the architecture.

Diff Detail

Event Timeline

JonChesterfield created this revision.Sep 1 2021, 5:55 AM
JonChesterfield requested review of this revision.Sep 1 2021, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2021, 5:55 AM
JonChesterfield edited the summary of this revision. (Show Details)Sep 1 2021, 5:56 AM
jdoerfert accepted this revision.Sep 1 2021, 9:08 AM

LG, but please add a test for this.

This revision is now accepted and ready to land.Sep 1 2021, 9:08 AM
JonChesterfield added a comment.EditedSep 1 2021, 9:17 AM

LG, but please add a test for this.

D109061 sends everything in check-openmp down this code path, but we should indeed have a check for the file and for the dir version. Will take a look

walked back through git to find https://reviews.llvm.org/D95161, should be able to do something similar to the tests added there

Well, that's not helpful. Managed to add a test through the webui as arcanist was misbehaving but it has worked by deleting the functional change. Will try again

OK, test added, same code as originally, checked that the test fails on trunk. Because it's driver only we don't get a far as the fatal error, but you can see the driver passing the directory through to builtin-bitcode. Will land when the premerge checks catch up.

This revision was landed with ongoing or failed builds.Sep 1 2021, 11:46 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Sep 1 2021, 12:20 PM

This breaks tests on windows: http://45.33.8.238/win/44930/step_7.txt

Please take a look and revert if it takes a while to fix. Probably just need to optionally accept win slashiness in the test.

JonChesterfield added a comment.EditedSep 1 2021, 12:33 PM

This breaks tests on windows: http://45.33.8.238/win/44930/step_7.txt

Please take a look and revert if it takes a while to fix. Probably just need to optionally accept win slashiness in the test.

Ah, that explains the {{.*}} patterns. Will fix by dropping the 'libomptarget/' string as the important part is that it found the sm_35.bc in that directory. Commit 88511f6bc56792b47

JonChesterfield added a comment.EditedSep 1 2021, 12:47 PM

Bad times. Changing that to accept the / was a shotgun fix for the problem but it collides with one of the nearby regex, reverting while I work out what is going on. Acceptable fix for linux is to replace the / with {{.}}, as that avoids the problem of two adjacent filecheck matches looking for the same string. I expect that to fix windows too, but will wait for the linked bot above to go through clean first.

JonChesterfield reopened this revision.Sep 1 2021, 12:47 PM
This revision is now accepted and ready to land.Sep 1 2021, 12:47 PM
This revision was automatically updated to reflect the committed changes.

Turns out {{.}} is also fine as far as Linux is concerned and unacceptable to Windows. I can't run anything on Windows locally, and trial and error while watching http://45.33.8.238/win has taken too many iterations already, so I've dropped the test case in 06cdf48a0d94749a19b0b.

Turns out {{.}} is also fine as far as Linux is concerned and unacceptable to Windows. I can't run anything on Windows locally, and trial and error while watching http://45.33.8.238/win has taken too many iterations already, so I've dropped the test case in 06cdf48a0d94749a19b0b.

@Meinersbur can you take a look?

Meinersbur added inline comments.Sep 8 2021, 10:24 AM
clang/test/Driver/openmp-offload-gpu.c
178

-### escapes backslashes in Windows paths UNIX-style (which actually is unnecessary/wrong, the cmd.exe escape character is ^, but windows also ignores consecutive slashes), so the actual output is

"-mlink-builtin-bitcode" "[...]\\clang\\test\\Driver/Inputs/libomptarget\\libomptarget-nvptx-sm_35.bc"

The regex {{.}} does not capture the double backslash. {{/|\\\\}} would work.

JonChesterfield added a comment.EditedSep 8 2021, 10:27 AM

Nice, thanks! I'll wait for the main CI to come back up before trying that

Works on linux as expected, landed - now watching http://45.33.8.238/summary.html

regex works, thanks!