This is an archive of the discontinued LLVM Phabricator instance.

Add HLSL Language Option and Preprocessor
ClosedPublic

Authored by beanz on Mar 19 2022, 3:55 PM.

Details

Summary

Bringing in HLSL as a language as well as language options for each of
the HLSL language standards.

While the HLSL language is unimplemented, this patch adds the
HLSL-specific preprocessor defines which enables testing of the command
line options through the driver.

Diff Detail

Event Timeline

beanz created this revision.Mar 19 2022, 3:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2022, 3:55 PM
Herald added a subscriber: dexonsmith. · View Herald Transcript
beanz requested review of this revision.Mar 19 2022, 3:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2022, 3:55 PM
pete accepted this revision.Mar 19 2022, 6:21 PM

LGTM

This revision is now accepted and ready to land.Mar 19 2022, 6:21 PM
rnk accepted this revision.Mar 23 2022, 11:04 AM

lgtm

clang/lib/Frontend/InitPreprocessor.cpp
401–403

This here suggests that the order of the HLSL environments in the triple really, really matter, since we expose them to users. That's really surprising: it means small changes in LLVM affect this pre-processor macro in surprising ways. I see the test for this, but what do you think about adding a static_assert to Triple.cpp to move the check into LLVM? Something like:

static_assert(Triple::Compute - Triple::Pixel == 1, "incorrect HLSL env order");
static_assert(Triple::Amplification - Triple::Pixel == 2, "incorrect HLSL env order");

Maybe this is addressed elsewhere, I haven't looked at all patches holistically.

beanz added inline comments.Mar 23 2022, 11:12 AM
clang/lib/Frontend/InitPreprocessor.cpp
401–403

This is more or less verified by the test case checking the preprocessor values, but static_asserts would provide a more clear error, so I'll add some.

This revision was landed with ongoing or failed builds.Mar 28 2022, 2:16 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald Transcript
erichkeane added inline comments.
clang/lib/Frontend/InitPreprocessor.cpp
401

Why does this do a double-assign here? Is th ere supposed to be a 2nd variable here? Otherwise this is likely not particularly well-defined behavior.

beanz added inline comments.Mar 29 2022, 10:45 AM
clang/lib/Frontend/InitPreprocessor.cpp
401

I must have screwed this up in rebating one of the patches. Will fix ASAP.