[SYCL] Fix libspirv invalid path in -###

[wenju-he]

In our downstream repo, bf21b9d exposed a regression that libspirv path is invalid in -### output. Before bf21b9d, the path was correctly found by searchAt(D.ResourceDir, "..", "..", "clc"), which was deleted by the commit.
Root cause is that in -### mode the process should not early return upon the first candidate, which may not exist. Instead, it should evaluate all candidates.

[Maetveis]

NAK.
You're effectively changing that the output of -### from the first candidate to the last one. I don't see why this would be anymore "correct" than the first.

The special handling of -### should just be removed and the tests that depend on it fixed (either by adding -ccc-install-dir %/S/Inputs/SYCL/bin or -fsycl-no-libspirv as appropriate). I had this in mind when doing the linked PR chain, but didn't get to it in the end.

-### is not a "testing" option, as it's description notes:

Print (but do not run) the commands to run for this compilation

(clang/include/clang/Driver/Options.td:721)

So it should print the exact same commands that would be run without -###, which this patch or the original code does not do. If we are going to fix this, lets do it properly.

[wenju-he]

You're effectively changing that the output of -### from the first candidate to the last one

-### isn't always using the last one. -### path uses the same libspirv file as when -### is not present, unless none of the candidates are valid.

[Maetveis]

You're effectively changing that the output of -### from the first candidate to the last one

-### isn't always using the last one. -### path uses the same libspirv file as when -### is not present, unless none of the candidates are valid.

When none of the candidates are valid we should just raise an error, just like normal compilation without -### does.

[wenju-he]

You're effectively changing that the output of -### from the first candidate to the last one

-### isn't always using the last one. -### path uses the same libspirv file as when -### is not present, unless none of the candidates are valid.

When none of the candidates are valid we should just raise an error, just like normal compilation without -### does.

this is a different issue than you raised before, right? and this issue exists before my PR.

[Maetveis]

this is a different issue than you raised before, right? and this issue exists before my PR.

No, this is the issue I meant by my first comment. Yes, you're correct that the issue is still there before this PR, but I believe it IS the same issue you're setting out to solve in the PR description:

"When -### is provided the printed commands do not contain the same path to libspirv as they would without -###."

The root cause of this is that we special case -###, when we shouldn't. When libspirv isn't available picking the last one instead of raising an error is just as wrong as picking the first. I get that you're trying to fix the case when it is available, and this PR is an improvement in that situation, but I'd rather see the issue fixed entirely instead of adding yet more workarounds to this hack.

If you really rather not do it, I can prepare a PR, the work mostly comes down to fixing (hopefully) a few clang driver tests.

[wenju-he]

When libspirv isn't available picking the last one instead of raising an error is just as wrong as picking the first.

Do you mean raise following error?

clang++: error: cannot find 'remangled-l64-signed_char.libspirv-nvptx64-nvidia-cuda.bc'; provide path to libspirv library via '-fsycl-libspirv-path', or pass '-fno-sycl-libspirv' to build without linking with libspirv

If the error is reported, then -### won't print anything to the screen. How is that useful for debugging?

I see another place that can use invalid path in -### mode:

llvm/clang/lib/Driver/ToolChains/ZOS.cpp

Lines 312 to 323 in f763de4

	void ZOS::TryAddIncludeFromPath(llvm::SmallString<128> Path,
	const llvm::opt::ArgList &DriverArgs,
	llvm::opt::ArgStringList &CC1Args) const {
	if (!getVFS().exists(Path)) {
	if (DriverArgs.hasArg(options::OPT_v))
	WithColor::warning(errs(), "Clang")
	<< "ignoring nonexistent directory \"" << Path << "\"\n";
	if (!DriverArgs.hasArg(options::OPT__HASH_HASH_HASH))
	return;
	}
	addSystemInclude(DriverArgs, CC1Args, Path);
	}

[wenju-he]

Current logic of outputting invalid libspirv path when none of the candidates exist does have a small benefit:
First, we try normal compilation command and it fails with error clang++: error: cannot find 'remangled-l64-signed_char
Then, we use -### output to find what libspirv the compiler is trying to looking at. This is useful for debugging.

[Maetveis]

Do you mean raise following error?

clang++: error: cannot find 'remangled-l64-signed_char.libspirv-nvptx64-nvidia-cuda.bc'; provide path to libspirv library via '-fsycl-libspirv-path', or pass '-fno-sycl-libspirv' to build without linking with libspirv

Yes.

If the error is reported, then -### won't print anything to the screen. How is that useful for debugging?

What would you be debugging in that case? The original command also raised that error, it also tells the user exactly what's wrong. If you mean that you want to debug something else than why libspirv is missing in a clang setup without libspirv built, then you can either pass -fno-sycl-libspirv or obtain libspirv from somewhere.

I see another place that can use invalid path in -### mode:

llvm/clang/lib/Driver/ToolChains/ZOS.cpp

Lines 312 to 323 in f763de4

	void ZOS::TryAddIncludeFromPath(llvm::SmallString<128> Path,
	const llvm::opt::ArgList &DriverArgs,
	llvm::opt::ArgStringList &CC1Args) const {
	if (!getVFS().exists(Path)) {
	if (DriverArgs.hasArg(options::OPT_v))
	WithColor::warning(errs(), "Clang")
	<< "ignoring nonexistent directory \"" << Path << "\"\n";
	if (!DriverArgs.hasArg(options::OPT__HASH_HASH_HASH))
	return;
	}
	addSystemInclude(DriverArgs, CC1Args, Path);
	}

When clang searches for other device libraries like cuda libdevice or ocml for AMDGPU also raise an error, in my opinion ZOS is also probably in the wrong, but I don't know enough context to be sure. @intel/dpcpp-clang-driver-reviewers can you please chime in if special casing -### as a "debug" option like this is reasonable?

[Maetveis]

Current logic of outputting invalid libspirv path when none of the candidates exist does have a small benefit: First, we try normal compilation command and it fails with error clang++: error: cannot find 'remangled-l64-signed_char Then, we use -### output to find what libspirv the compiler is trying to looking at. This is useful for debugging.

FTFY: After not finding any of the candidates the compiler pretends that it found it at the last location it would search at, and print that and only that location.

Even though -### is commonly used for testing and debugging, it is NOT a testing / debugging option. It's effect should be to "Print (but do not run) the commands to run for this compilation", nothing more. If you feel there should be more visibility into where the compiler was looking for to find libspirv, I wouldn't mind that, but it shouldn't be tied to -###.

[wenju-he]

What would you be debugging in that case?
Even though -### is commonly used for testing and debugging

As you said, I normally using -### for debugging.

[Maetveis]

What would you be debugging in that case?
Even though -### is commonly used for testing and debugging

As you said, I normally using -### for debugging.

I have nothing to add to this, I have stated my opinion in the comment you're referencing. Let's wait for the driver reviewers opinion on the topic, it is ultimately what matters :)