[PATCH] D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities

Fri Apr 8 10:05:22 PDT 2022

MaskRay added inline comments.

================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:380
+  switch (e) {
+  case ExecutionMode::Invocations:
+    return "Invocations";
----------------
MaskRay wrote:
> iliya-diyachkov wrote:
> > MaskRay wrote:
> > > Can we use X macros for the long list?
> > Initially all these functions were generated using macros. Then Renato suggested getting rid of them and we agreed to use expanded macros for now. Later we'll re-implement all the functions with tablegen (this work is in progress so I expect it to be completed in several months).
> > 
> > I have already fixed a number of issues in these functions after expanding the macros. I'm not sure that now we really need to replace them with new macros.
> Ah, ok. @rengolin I feel that X macros may be more readable and less error-prone than open coding here.
> 
> TableGen can generate such strings as well, but it will add one step, bring a little abstraction code, and is probably not conciser than macros here (AIUI one line for one enumeration value). WDYT?
The recent clang-format discussion is also relevant here. I think the current open coding can be perceived as error-prone by some folks: 
https://discourse.llvm.org/t/enable-single-line-case-statements-in-llvm-clang-format-style/61062

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

https://reviews.llvm.org/D116464