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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:380
+  switch (e) {
+  case ExecutionMode::Invocations:
+    return "Invocations";
----------------
MaskRay wrote:
> 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
> 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

As I know we'll have all these functions fully generated by TableGen, so it will be quite concise.

In the first version we had quite complex macros for all BaseInfo entities, and even more complex macros in the original Khronos repository, so we shouldn't return to that implementation.

I assume that you are now suggesting using very simple macros like this:
```
#define CASE(CLASS, MEMBER) \
  case CLASS::MEMBER: return #MEMBER;
...
StringRef SPIRV::getExecutionModeName(ExecutionMode e) {
  switch (e) {
  CASE(ExecutionMode, Invocations)
```
I'm not sure whether we need this at the moment, so I set TODO before all the functions for now.

However if you think it's a better solution then we have now, no problem, I'll change all the functions (with this open coding). But we all need to agree on this.


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

https://reviews.llvm.org/D116464