[PATCH] D123278: [Clang] [Docs] Add HLSLSupport page

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Looks good to me overall, I just left some local comments. Please take my writing suggestions with a pinch of salt, English is my second language.



================
Comment at: clang/docs/HLSLSupport.rst:13
+describes the high level goals of the project, the guiding principals, as well
+as some of the idiosyncrasies of the HLSL language and how we intend to support
+them in Clang.
----------------
ubernit: some of the idiosyncrasies -> some idiosyncrasies


================
Comment at: clang/docs/HLSLSupport.rst:22-23
+<https://github.com/microsoft/DirectXShaderCompiler/>`_. in all its supported
+use cases. Accomplishing that goal will require a high level of source
+compatibility.
+
----------------
What do you mean by source compatibility in this case? In terms of the input source language (HLSL), or the source code of both implementations?

Perhaps we could make it more direct? E.g., 'This requires us to add the HLSL language support to Clang.'


================
Comment at: clang/docs/HLSLSupport.rst:28-31
+HLSL ASTs do not need to be compatible between DXC and Clang. We do not expect
+identical code generation or that features will resemble DXC's implementation or
+architecture. In fact, we explicitly expect to deviate from DXC's implementation
+in key ways.
----------------
It's great you made this so explicit!


================
Comment at: clang/docs/HLSLSupport.rst:36
+
+This document lacks precise details for architectural decisions that are not yet
+finalized. Our top priorities are quality, maintainability, and flexibility. In
----------------
ubernit: how about just 'details'?


================
Comment at: clang/docs/HLSLSupport.rst:50-52
+HLSL is not a formally or fully specified language, and while our goals require
+a high level of source compatibility, implementations can vary and we have some
+flexibility to be more or less permissive in some cases.
----------------
Is DXC the reference implementation?


================
Comment at: clang/docs/HLSLSupport.rst:55-56
+The HLSL effort prioritizes following similar patterns for other languages,
+drivers, runtimes and targets. Specifically, HLSL will create separate
+implementation files to contain the HLSL-specific behavior wherever it makes
+sense (i.e. ParseHLSL.cpp, SemaHLSL.cpp, CGHLSL*.cpp...). We will use inline
----------------
This is a bit awkward, as if the language HSLS created files on disk on something :P. 

Maybe something like: We will maintain separation between HSLS-specific code and the rest of Clang as much as possible (e.g., ParseHSLS.cpp, ...)


================
Comment at: clang/docs/HLSLSupport.rst:90-91
+metadata. *hand wave* *hand wave* *hand wave* As a design principle here we want
+our IR to be idiomatic Clang IR as much as possible. We will use IR attributes
+wherever we can, and use metadata as sparingly as possible. One example of a
+difference from DXC already implemented in Clang is the use of target triples to
----------------
Are all attributes guaranteed to be preserved? I thought some might get dropped by opt.


================
Comment at: clang/docs/HLSLSupport.rst:128
+HLSL does support member functions, and (in HLSL 2021) limited operator
+overloading. With member function support HLSL also has a ``this`` keyword. The
+``this`` keyword is an example of one of the places where HLSL relies on
----------------
nit: With member function support*,* HLSL also


================
Comment at: clang/docs/HLSLSupport.rst:135
+
+This is a simple one, but it deviates from C so is worth mentioning. HLSL
+bitshifts are defined to mask the shift count by the size of the type. In DXC,
----------------
nit: so *it* is worth?


================
Comment at: clang/docs/HLSLSupport.rst:137
+bitshifts are defined to mask the shift count by the size of the type. In DXC,
+the semantics of LLVM IR were altered to accomodate this, in Clang we intend to
+generate the mask explicitly in the IR.
----------------
accommodate


================
Comment at: clang/docs/HLSLSupport.rst:187
+
+HLSL does not support the following C++ features:
+
----------------
These are C++ language features. I assume that all library features are also out of the window?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123278