[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:19
+
+using namespace llvm;
+using namespace SPIRV;
----------------
rengolin wrote:
> why not use:
>    namespace llvm {
>       namespace SPIRV {
> 
> and then avoid prefixing all types with `SPIRV`?
You are right, we can avoid `SPIRV` prefix.


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:380
+  switch (e) {
+  case ExecutionMode::Invocations:
+    return "Invocations";
----------------
rengolin wrote:
> iliya-diyachkov wrote:
> > 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.
> I honestly can't remember what was there before, but this huge list of strings does seem a bit out of hand.
> 
> With macros, we'd still have a huge list of calls, but we wouldn't have the opportunity to make typo mistakes (if using `X(str) case str: return "#str";`).
> 
> I'm also not sure why you need all of these strings at this level, unless you want to print all that in your assembly output. Is that the point? Or is it just debug?
> With macros, we'd still have a huge list of calls, but we wouldn't have the opportunity to make typo mistakes (if using X(str) case str: return "#str";).
Ok, I'll change this to X macros.

>I'm also not sure why you need all of these strings at this level, unless you want to print all that in your assembly output. Is that the point?
Yes, we need to print all these operands in the assembly output. Anyway the TableGen version is in progress, so this huge list will be removed.


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

https://reviews.llvm.org/D116464