[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:776
+      break;
+    uint32_t Imm = Op.getImm(); // Each i32 word is up to 4 characters
+    for (unsigned ShiftAmount = 0; ShiftAmount < 32; ShiftAmount += 8) {
----------------
MaskRay wrote:
> May consider `assert` instead of a comment
Ok, "assert((Op.getImm() >> 32) == 0..." will be added.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h:9-12
+// SPIRVGlobalRegistry is used to maintain rich type information required for
+// SPIR-V even after lowering from LLVM IR to GMIR. It can convert an llvm::Type
+// into an OpTypeXXX instruction, and map it to a virtual register. Also it
+// builds and supports consistency of constants and global variables.
----------------
arsenm wrote:
> Why do you need to do this? What are you losing in gMIR that matters?
SPIR-V annotates each instruction with type IDs. SPIR-V uses high-level types including composites (structs, arrays...) and some others. Most of them can be obtained from LLVM Types but they are not available in gMIR, so we need to save and maintain own type information.



================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:518
+  assert(I.hasOneMemOperand());
+  auto MemOp = *I.memoperands_begin();
+  auto Scope = getScope(MemOp->getSyncScopeID());
----------------
arsenm wrote:
> MaskRay wrote:
> > The file may have too many `auto`. See https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> I agree there are too many autos
The autos will be cleaned.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:835-843
+    if (STI.canDirectlyComparePointers()) {
+      std::tie(CmpOpc, PtrToUInt) = getPtrCmpOpcode(Pred);
+    } else {
+      PtrToUInt = true;
+    }
+  } else if (GR.isScalarOrVectorOfType(CmpOperand, SPIRV::OpTypeBool)) {
+    TypeOpc = SPIRV::OpTypeBool;
----------------
arsenm wrote:
> It looks like you're repeating work done in the legalizer?
Yes, it looks like I need to fix this. 


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:1138
+  auto GlobalIdent = GV->getGlobalIdentifier();
+  auto GlobalVar = dyn_cast<GlobalVariable>(GV);
+
----------------
arsenm wrote:
> Should just use cast<> and remove the check, this should never fail. I believe we disallow null G_GLOBAL_VALUE sources
Ok, I'll check this.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:29
+
+static const std::unordered_set<unsigned> TypeFoldingSupportingOpcs = {
+    TargetOpcode::G_ADD,
----------------
MaskRay wrote:
> There is a non-trivial ctor. If you iterate it, the order is not deterministic (https://llvm.org/docs/ProgrammersManual.html).
It will be replaced by std::set.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVUtils.cpp:132-133
+    return StorageClass::Workgroup;
+  case 4:
+    return StorageClass::Generic;
+  case 7:
----------------
arsenm wrote:
> How/why are you not using generic as addrspace 0?
One of the important features of this backend is compatibility with SPIRV-LLVM-Translator. We follow the address spaces agreement announced in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst#address-spaces . 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464