[PATCH] D122087: Add HLSL Language Option and Preprocessor

Wed Mar 23 11:04:19 PDT 2022

rnk accepted this revision.
rnk added a comment.
Herald added a subscriber: StephenFan.

lgtm

================
Comment at: clang/lib/Frontend/InitPreprocessor.cpp:401-403
+    uint32_t StageInteger = StageInteger =
+        (uint32_t)TI.getTriple().getEnvironment() -
+        (uint32_t)llvm::Triple::Pixel;
----------------
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122087/new/

https://reviews.llvm.org/D122087