[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.h:23
+
+#define MAKE_ENUM(Enum, Var, Val) Var = Val,
+
----------------
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.



================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:81
+                                                MachineIRBuilder &MIRBuilder) {
+  using namespace SPIRV;
+  auto EleOpc = ElemType->getOpcode();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464