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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

This is a much larger patch and I don't know the target well enough. I have a few general comments inline but would be nice if someone that knows the target better to have a look?



================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h:23
+
+#define MAKE_ENUM(Enum, Var, Val) Var = Val,
+
----------------
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).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464