[PATCH] D122087: Add HLSL Language Option and Preprocessor

Wed Mar 23 11:12:19 PDT 2022

beanz added inline comments.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122087