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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:43
 class SPIRVAsmPrinter : public AsmPrinter {
+  const MCSubtargetInfo *STI;
 
----------------
arsenm wrote:
> Might as well use SPIRVSubtarget?
You probably mean removing STI and using OutContext.getSubtargetInfo(). 


================
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();
----------------
arsenm wrote:
> getSubtarget<SPIRVSubtarget>() instead of static_cast
As I see we need to have either MachineFunction or Function to call getSubtarget<SPIRVSubtarget>().
However in SPIRVAsmPrinter ST is required even if there are no functions in the module. I don't see any other way than to use static_cast as some other targets do (e.g. in NVPTXAsmPrinter::emitStartOfAsmFile or in AVRAsmPrinter::emitStartOfAsmFile). If you know, please advise.


================
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;
+
----------------
arsenm wrote:
> Can't have global constructors
I'll move BBNumToRegMap into struct ModuleAnalysisInfo.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:141
+// Convert MBB's number to correspnding ID register.
+Register getOrCreateMBBRegister(const MachineBasicBlock &MBB,
+                                ModuleAnalysisInfo *MAI) {
----------------
arsenm wrote:
> static?
getOrCreateMBBRegister() was not static since it was called in SPIRVMCInstLower.cpp (outside of this module). I'll make getOrCreateMBBRegister() a member of struct ModuleAnalysisInfo as well as BBNumToRegMap.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:165
+        return;
+    llvm_unreachable("OpFunction is expected in the front MBB of MF");
+  }
----------------
arsenm wrote:
> If this isn't checked by the verifier (which would be preferable), should be report_fatal_error
I believe that it is not checked. I'll add the appropriate TODO and change llvm_unreachable to report_fatal_error.


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


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

https://reviews.llvm.org/D116465