[HLSL] [Dirver] add dxv as a Driver Action Job

[python3kgae]

Summary

New option --dxv-path is added for dxc mode to set the installation path for dxv.
If cannot find dxv, a warning will be report.

dxv will be executed with command line dxv file_name -o file_name.
It will validate and sign the file and overwrite it.

[beanz]

Re-using the VerifyDebug action really doesn't make sense. That's not what the DXIL validator does, and it will be a source of confusion forever.

[beanz]

clang/lib/Driver/Driver.cpp

4215

Shouldn't the validator only run if we are targeting DXIL? Also we should probably add the -Vd flag to opt out.

clang/lib/Driver/ToolChains/HLSL.cpp

165

If this option isn't specified we should probably search for dxv relative to clang and on the PATH.

[python3kgae]

Add BinaryAnalyzeJobClass.
Add -Vd to disable validation.

[python3kgae]

In D141705#4059162, @beanz wrote:

Re-using the VerifyDebug action really doesn't make sense. That's not what the DXIL validator does, and it will be a source of confusion forever.

Added BinaryAnalyzeJobClass.

[jhuber6]

I'm not overly familiar with HLSL or DirectX here. Most of the changes are purely mechanical, but I don't see anywhere we create the tool. Does that come later? Normally you'd test these with -ccc-print-bindings, -ccc-print-bindings, and -###.

clang/lib/Driver/ToolChains/HLSL.cpp

146–147

Can we really just ignore this if we don't find the path? Normally this is a hard error. If this is purely optional I would probably suggest not creating the job in the first place.

170

Copy paste.

clang/test/Misc/warning-flags.c

19 ↗

(On Diff #489935)

This comment looks relevant.

[beanz]

clang/include/clang/Basic/DiagnosticDriverKinds.td

701

Not all dxv binaries can sign… some just validate. Only the “official” releases support signing.

[python3kgae]

Only create dxv job when 'dxv' exist.
Test ccc-print-bindings.
Code cleanup.

[python3kgae]

In D141705#4082935, @jhuber6 wrote:

I'm not overly familiar with HLSL or DirectX here. Most of the changes are purely mechanical, but I don't see anywhere we create the tool. Does that come later? Normally you'd test these with -ccc-print-bindings, -ccc-print-bindings, and -###.

The tool ('dxv') is not included in hlsl compiler release yet. It will come later.
Added test with ccc-print-bindings.

clang/include/clang/Basic/DiagnosticDriverKinds.td

701

Fixed.

clang/lib/Driver/ToolChains/HLSL.cpp

146–147

This is optional.
I'll move the check when create the job.

170

Good catch.
Fixed.

clang/test/Misc/warning-flags.c

19 ↗

(On Diff #489935)

Fixed.

[jhuber6]

clang/lib/Driver/Driver.cpp

4216

Could we move this logic into the HLSL ToolChain like we do with CUDA / ROCm? You should be able to then cast the toolchain to HLSLToolChain and query it here.

4217

4222

Why is the type TY_Nothing? The ConstructJob invocation shows it using -o. So it should have output?

4224

clang/lib/Driver/ToolChains/HLSL.cpp

171

Just to check since I'm not really familiar at all with this toolchain, but does dxv exist as a clang tool? This path exists to search in the /path/to/llvm/install/bin directory. If it's an external binary this shouldn't be necessary.

clang/test/Driver/dxc_dxv_path.hlsl

12

Lines that check clang typically start with "-cc1"

15–16

Can we get a test for -ccc-print-phases as well? Also, I wouldn't bother checking the test file's name and it's good practice to stipulate NEXT on these kinds of tests.

[python3kgae]

add DX_CONTAINER Driver type.
Code cleanup.

[python3kgae]

clang/lib/Driver/Driver.cpp

4216

Cannot find a cast example :(
Created a static method for HLSLToolChain to put the logic.

clang/lib/Driver/ToolChains/HLSL.cpp

171

It will be external binary for a long time.
Removed the getDriver().Dir.

[beanz]

clang/include/clang/Driver/Types.def

110

The normal dx-container extension is dxbc right? not dxc?

[jhuber6]

clang/lib/Driver/Driver.cpp

4229–4230

4233

This should work, shouldn't it?

const auto &TC = static_cast<const toolchains::HLSLToolChain &>(getToolChain());

clang/test/Driver/dxc_dxv_path.hlsl

15–16

nit

20–24

[python3kgae]

clang/lib/Driver/Driver.cpp

4233

I'm not sure getToolChain will return a HLSLToolChain.
Is it OK to use dynamic_cast?

[jhuber6]

clang/lib/Driver/Driver.cpp

4233

Well, how is the compilation normally invoked? The Driver::getToolChain() function clearly returns an HLSLToolChain if the triple is ShaderModel. I'm assuming that's the only way to create this, and its set in the compilation. So it should be the ToolChain of the main compilation. This is in contract to an offloading compilation that can have multiple toolchains floating around I'm guessing.

And no, dynamic_cast doesn't work in LLVM. LLVM RTTI requires implementing a classof function, e.g. https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html, which the ToolChains don't use.

clang/test/Driver/dxc_dxv_path.hlsl

20–24

Also get rid of the +-.

[python3kgae]

clang/include/clang/Driver/Types.def

110

There's no official extension.
Will go with dxo.

clang/lib/Driver/Driver.cpp

4233

const auto &TC = static_cast<const toolchains::HLSLToolChain *>(
    getToolChain(Args, C.getDefaultToolChain().getTriple()));

will hit " no viable conversion from 'const clang::driver::ToolChain' to 'const toolchains::HLSLToolChain'"

const auto &TC = getToolChain(Args, C.getDefaultToolChain().getTriple());
const auto *HLSLTC = static_cast<const toolchains::HLSLToolChain *>(&TC);

works, is it OK?

[python3kgae]

cast ToolChain to HLSLToolChain.

[jhuber6]

clang/lib/Driver/Driver.cpp

4232–4233

This can create a new ToolChain. We probably don't want that. Maybe someone more familiar with this compilation mode can chime in. But my guess is that when we created the Compilation.

This code will only fire if the above triple matches for the default ToolChain. So we should be safe in just casting it directly.

clang/lib/Driver/ToolChains/HLSL.cpp

258

We don't need to pass in D since this is no longer static. You can use getDriver()

clang/test/Driver/dxc_dxv_path.hlsl

20

nit. remove the +- and add whitespace to make it more readable.

[python3kgae]

switch to getDefaultToolChain.

[python3kgae]

clang/lib/Driver/Driver.cpp

4232–4233

Switched to getDefaultToolChain.

[jhuber6]

Thanks for the changes.

clang/lib/Driver/Driver.cpp

4241

nit. remember to clang-format

clang/lib/Driver/ToolChains/HLSL.cpp

170

I feel like this logic should go with the other random Tool versions we have in ToolChain.cpp. See ToolChain.cpp:440 and there should be examples of similar tools.

clang/lib/Driver/ToolChains/HLSL.h

53

Is needValidation a good name here? It's asking more like hasValidator or supportsValidation.

[python3kgae]

clang/lib/Driver/Driver.cpp

4241

arc did find some clang-format issue.
But missed this one :(

clang/lib/Driver/ToolChains/HLSL.cpp

170

This is following pattern of MachO::getTool for VerifyDebug.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Darwin.cpp#L1078

I can add getValidator to HLSLToolChain if that helps.

clang/lib/Driver/ToolChains/HLSL.h

53

It is combination of hasValidator and requireValidator.
Maybe create hasValidator and requireValidator then move the warning reporting back to Driver?
Something like

if (TC.requireValidator()) {
    if (TC.hasValidator()) {
        add the action.
    } else {
        report warning.
    }
}

[jhuber6]

clang/lib/Driver/ToolChains/HLSL.cpp

170

Yeah, I guess it's a style thing. Personally I don't mind having everything in one place because you need to handle the ActionClass there anyway. But it's not a huge deal. I'll accept either.

clang/lib/Driver/ToolChains/HLSL.h

53

I think keeping the logic as-is and calling it requiresValidation would be fine.

[python3kgae]

rename to requiresValidation.

[python3kgae]

clang/lib/Driver/ToolChains/HLSL.cpp

170

Thanks. I'll keep it as is then.
The validator is only for HLSL now.
If other ToolChain uses it, I'll move it to ToolChain.

[jhuber6]

LGTM overall. Thanks for the patch. Others feel free to comment.

clang/lib/Driver/Driver.cpp

4214–4226

Extreme nit. whitespace above.

clang/test/Driver/dxc_dxv_path.hlsl

8

Would this be called dxv.exe on Windows?

[python3kgae]

Fix windows test fail.

[python3kgae]

clang/test/Driver/dxc_dxv_path.hlsl

8

Good catch.
Changed to dxv{{(.exe)?}}"

Thanks a lot for the review.

[MaskRay]

clang/include/clang/Basic/DiagnosticDriverKinds.td

701

Make sure there is a space after .. The diagnostics convention does not include the trailing period.

[python3kgae]

add space for warning message.

clang/include/clang/Basic/DiagnosticDriverKinds.td

701

Good catch!
Fixed.