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

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h:23
+
+#define MAKE_ENUM(Enum, Var, Val) Var = Val,
+
----------------
iliya-diyachkov wrote:
> rengolin wrote:
> > We generally frown upon pre-processor macros to define code as it can be really hard to understand what's going on, especially during debug.
> > 
> > Looking at other targets' `BaseInfo`, none of them are as complicated as this one, so I'm not sure what to say, really.
> > 
> > Another problem perhaps is that these macros will be available to any header or source file that include `BaseInfo`, which can be many, and can encourage people using it elsewhere (hopefully, only on this target).
> Well, I agree that it looks unpleasant and a little confusing and it can complicate debugging.
> 
> In fact, these macros define 30 enums representing entities specific to SPIR-V, and name-getters for them, like the following:
> ```
> namespace SourceLanguage {
> enum SourceLanguage : uint32_t {
>   Unknown = 0,
>   ESSL = 1,
>   GLSL = 2,
>   OpenCL_C = 3,
>   OpenCL_CPP = 4,
>   HLSL = 5,
> };
> }
> std::string getSourceLanguageName(SourceLanguage::SourceLanguage e);
> ```
> The name getters are declared in SPIRVBaseInfo.cpp.
> 
> These enums are widely used in the general SPIRV code and in several passes, and we can't just get rid of them. We can declare them all manually without macros, but this will increase the source code and probably reduce maintainability/extensibility.
> 
> Actually these are simplified macros for SPIRV-specific entities. We have several other source files (I have not published them in this series) that use more complex version of these macros, e.g. they include features, extensions and SPIR-V versions specific to each particular enum member. Like this:
> ```
>  X(N, StorageUniform16, 4434, {SBBA(16)}, {SPV_16_BIT}, 0x10300, 0)
> ```
> It may be convenient for maintainers to have all info about each entity in one place.
> 
> I admit that some of these macros can be re-implemented using tablegen, but for the first time I would leave existing approach, perhaps with some improvements.
> 
> I see that some of the macro definitions can be moved from SPIRVBaseInfo.h to SPIRVBaseInfo.cpp so they will not affect other sources that include SPIRVBaseInfo.h.
> Also (if it helps somehow) we could separate this BaseInfo* to LLVMSPIRVUtils lib, as it's done in AArch64, AMDGPU, ARM and WebAssembly targets.
> 
That is certainly the right place for those enums, just not defined via complex chain of macros.

If you have more of them, and even more complex, downstream, it would certainly be the right time now to move them to TableGen, otherwise it will be harder and harder to get rid of them in some complex refactoring pass.

This is one of the crucial parts of the first review, to make sure the code doesn't have "contentious issues". Macros in this way are a good candidate.

When the debugger hits that line, it will point to the macro definition, and will provide no recourse to looking at their definitions or expanding them. Worse, if we start using macros in other macros, the compiler will have a hard time telling the developers what went wrong in the generated code.

If you look around the code you'll notice that most macros are either silly things or around table-generated stuff. Table gen isn't perfect, but both debuggers and compilers can point at the generated files and you can step through or understand the error messages as any other source file.

In the meantime, have you tried looking at the pre-processed file generated from that header? Perhaps you could keep it expanded until you move to table-gen?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:81
+                                                MachineIRBuilder &MIRBuilder) {
+  using namespace SPIRV;
+  auto EleOpc = ElemType->getOpcode();
----------------
iliya-diyachkov wrote:
> rengolin wrote:
> > I assume this is to avoid typing `SPIRV::` a lot, but it can also shadow other namespaces and be confusing or even silently wrong.
> > 
> > If any of this code is copy and pasted elsewhere, it could also be dangerous, as the compiler might pick other namespaces and not emit errors until it's too late.
> > 
> > The risk is low, I agree, but the effort is also once.
> > 
> > Because of that, I'd recommend you just repeat the namespace name where its due to be abundantly clear which type you mean.
> > 
> > There are other cases in this patch below that would need to be changed, too.
> Ok, these namespace names will be placed expressly as required.
> 
> I suppose it's acceptable to use "using namespace TargetOpcode" in SPIRVLegalizerInfo::SPIRVLegalizerInfo() as it's done in AMDGPU, ARM and Mips targets.
I assume so, as it's widely used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464