This is an archive of the discontinued LLVM Phabricator instance.

[DirectX backend] set target triple to "dxil-ms-dx"
ClosedPublic

Authored by python3kgae on Aug 10 2022, 12:30 AM.

Details

Summary

Set target triple to "dxil-ms-dx" for DXIL at the end of DXILTranslateMetadata.

Diff Detail

Event Timeline

python3kgae created this revision.Aug 10 2022, 12:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 12:30 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
python3kgae requested review of this revision.Aug 10 2022, 12:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 12:30 AM

So... this is actually going to be terrible, but we can't do it this way. We rely on the module having a sane triple through MC emission.

What we need to do is change the triple right before DXIL bitcode generation, then change it back so that it is correct through MC.

Only set "dxil-ms-dx" TargetTriple for the output bitcode.

beanz added inline comments.Aug 19 2022, 8:09 AM
llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
54 ↗(On Diff #451921)

This is a bit confusingly named. Probably worth calling this SMVer instead and explaining in a comment that the DXIL and SM minor versions are the same, and the major versions are offset.

75 ↗(On Diff #451921)

nit: Rather than static const constexpr. The later is guaranteed to be compile-time resolved, the former isn't.

85 ↗(On Diff #451921)
86 ↗(On Diff #451921)

This one really requires you to know the API to know that getMinor() returns an Optional rather than a value.

Other places in LLVM use a pattern more like:

unsigned Minor = V.getMinor().value_or(0);

Which would also be more clear.

llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.h
78 ↗(On Diff #451921)

I know we had talked about where to mutate the triple and I thought WriteDXILToFile made sense, but seeing the code now that we lose the const-ness here I'm rethinking.

We only really need the triple to be altered when we're embedding DXIL in the module for emission of a container. I don't even think the WriteDXILPass is ever used (unless it were called manually).

It might make more sense to mutate the triple in EmbedDXILPass::runOnModule to preserve the const-ness of the writing function. We can also in a subsequent patch delete the WriteDXILPass.

python3kgae marked 5 inline comments as done.

Move triple update into EmbedDXILPass.

Rebase and fix test fail.

Rebase and only set target triple to "dxil-ms-dx"

python3kgae retitled this revision from [DirectX backend] emit metadata for DXIL version, ShaderModel. to [DirectX backend] set target triple to "dxil-ms-dx".Oct 13 2022, 5:22 PM
python3kgae edited the summary of this revision. (Show Details)
beanz accepted this revision.Oct 24 2022, 9:06 AM

Please add a check line to the embed-dxil.ll test to verify that the triple is restored after embedding the DXIL global, otherwise LGTM.

This revision is now accepted and ready to land.Oct 24 2022, 9:06 AM

Make sure triple is restored.

This revision was landed with ongoing or failed builds.Oct 24 2022, 2:49 PM
This revision was automatically updated to reflect the committed changes.