[PATCH] D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:43
 class SPIRVAsmPrinter : public AsmPrinter {
+  const MCSubtargetInfo *STI;
 
----------------
Might as well use SPIRVSubtarget?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:49
+      : AsmPrinter(TM, std::move(Streamer)), STI(TM.getMCSubtargetInfo()) {
+    ST = static_cast<const SPIRVTargetMachine &>(TM).getSubtargetImpl();
+    TII = ST->getInstrInfo();
----------------
getSubtarget<SPIRVSubtarget>() instead of static_cast


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:122
+// The table maps MBB number to SPIR-V unique ID register.
+static DenseMap<int, Register> BBNumToRegMap;
+
----------------
Can't have global constructors


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:140
+
+// Convert MBB's number to correspnding ID register.
+Register getOrCreateMBBRegister(const MachineBasicBlock &MBB,
----------------
Typo correspnding


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:141
+// Convert MBB's number to correspnding ID register.
+Register getOrCreateMBBRegister(const MachineBasicBlock &MBB,
+                                ModuleAnalysisInfo *MAI) {
----------------
static?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:165
+        return;
+    llvm_unreachable("OpFunction is expected in the front MBB of MF");
+  }
----------------
If this isn't checked by the verifier (which would be preferable), should be report_fatal_error


================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:161
+    MachineFunction *MF = MMI->getMachineFunction(*F);
+    if (MF) {
+      unsigned FCounter = 0;
----------------
I don't think this can fail at this point? If not, should early continue or assert and reduce indentation


================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:200
+    MachineFunction *MF = MMI->getMachineFunction(*F);
+    if (MF) {
+      for (MachineBasicBlock &MBB : *MF) {
----------------
Ditto


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

https://reviews.llvm.org/D116465