Fix for NVPTX module asm regression

[jdtatz]

Summary

When the new NVPTX backend was added in commit ae556d3ef72dfe5f40a337b7071f42b7bf5b66a4 the NVPTXAsmPrinter::doInitialization method did not call it's parent AsmPrinter::doInitialization, which would emit module-level inline asm if it existed.

Emitting of module-level inline asm was then added in commit d2bbdf05e0b88524226589d89ffb2bfdc53ef3c8 but this was done without calling AsmPrinter::doInitialization and instead fixed by copying the module-level asm emitting code into NVPTXAsmPrinter::doInitialization after calling NVPTXAsmPrinter::emitHeader

Calling AsmPrinter::doInitialization was then added to NVPTXAsmPrinter::doInitialization in commit 68f2218e1efdb74442987a2040876e2e41f7d90e but NVPTXAsmPrinter::emitHeader was called afterwards, which meant that any module-level inline asm was added before the header which results in invalid ptx.
Also the copied module-level inline asm emitting code was left in, so module-level inline asm was added to the final ptx file twice, once before the header during AsmPrinter::doInitialization in an invalid position and once again after the header during NVPTXAsmPrinter::doInitialization in the correct position.

I have fixed this error by removing the copied module-level inline asm emitting code and adding the NVPTXAsmPrinter::emitHeader call to a new NVPTXAsmPrinter instantiation of the virtual method AsmPrinter::emitStartOfAsmFile which was designed for this use case. I did not add a unit test as one was added in commit d2bbdf05e0b88524226589d89ffb2bfdc53ef3c8 but without calling ptxas -v on every emitted ptx file we can't not verify the correctness, so I'm not sure of the usefulness of the unit test other than to ensure it doesn't cause any internal llvm issues.

[tra]

Could you post a trivial example of where the module-level inline assembly is emitted before/after this patch?

Testing things that require ptxas would need to be done in the LLVM test-suite. CUDA buildbot is offline at the moment, but I'm working on getting a new one running soon.

[jdtatz]

Using the unit test

target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v16:16:16-v32:32:32-v64:64:64-v128:128:128-n16:32:64"
target triple = "nvptx-nvidia-cuda"

module asm ".global .b32 val;"

define void @foo() {
  ret void
}

with llvm-10 invalid ptx is generated

                                        // Start of file scope inline assembly
.global .b32 val;

                                        // End of file scope inline assembly
//
// Generated by LLVM NVPTX Back-End
//

.version 3.2
.target sm_20
.address_size 32

                                        // Start of file scope inline assembly
.global .b32 val;

                                        // End of file scope inline assembly
	// .globl	foo             // -- Begin function foo
                                        // @foo
.visible .func foo()
{


// %bb.0:
	ret;
                                        // -- End function
}

with this patch valid ptx is generated

//
// Generated by LLVM NVPTX Back-End
//

.version 3.2
.target sm_20
.address_size 32

                                        // Start of file scope inline assembly
.global .b32 val;

                                        // End of file scope inline assembly
	// .globl	foo             // -- Begin function foo
                                        // @foo
.visible .func foo()
{


// %bb.0:
	ret;
                                        // -- End function
}

[tra]

Thank you for providing the examples.

In D82280#2110206, @jdtatz wrote:

with llvm-10 invalid ptx is generated

                                        // Start of file scope inline assembly
.global .b32 val;

                                        // End of file scope inline assembly
//
// Generated by LLVM NVPTX Back-End
//

.version 3.2
.target sm_20
.address_size 32

                                        // Start of file scope inline assembly
.global .b32 val;

Ouch. We've generated it twice, with one of the instances being in the wrong place. That's indeed not good.

with this patch valid ptx is generated

//
// Generated by LLVM NVPTX Back-End
//

.version 3.2
.target sm_20
.address_size 32

                                        // Start of file scope inline assembly
.global .b32 val;

I like this much better. :-)

[jdtatz]

I don't have write access, could you please commit it for me?

[tra]

Will do.

[tra]

I did not add a unit test as one was added in commit d2bbdf05e0b88524226589d89ffb2bfdc53ef3c8 but without calling ptxas -v on every emitted ptx file we can't not verify the correctness

Before I land the test, we do need to fix test/CodeGen/NVPTX/module-inline-asm.ll as it did not detect the current issue.
The test currently only verifies the presence of the module-level inline assembly in the output, but it does not verify that it's in the right place or that it's only emitted once.

We heed few more checks that the inline assembly is only emitted after the initial PTX directives and before the functions.

[tra]

Updated the test.

llvm/lib/Target/NVPTX/NVPTXAsmPrinter.h

class LLVM_LIBRARY_VISIBILITY NVPTXAsmPrinter : public AsmPrinter {
   friend class AggBuffer;
 
 private:
   StringRef getPassName() const override { return "NVPTX Assembly Printer"; }
 
   const Function *F;
   std::string CurrentFnName;
 
+  void emitStartOfAsmFile(Module &M) override;
   void emitBasicBlockStart(const MachineBasicBlock &MBB) override;
   void emitFunctionEntryLabel() override;
   void emitFunctionBodyStart() override;
   void emitFunctionBodyEnd() override;
   void emitImplicitDef(const MachineInstr *MI) const override;
 
   void emitInstruction(const MachineInstr *) override;
   void lowerToMCInst(const MachineInstr *MI, MCInst &OutMI);

llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp

 
 static bool isEmptyXXStructor(GlobalVariable *GV) {
   if (!GV) return true;
   const ConstantArray *InitList = dyn_cast<ConstantArray>(GV->getInitializer());
   if (!InitList) return true;  // Not an array; we don't know how to parse.
   return InitList->getNumOperands() == 0;
 }
 
-bool NVPTXAsmPrinter::doInitialization(Module &M) {
+void NVPTXAsmPrinter::emitStartOfAsmFile(Module &M) {
   // Construct a default subtarget off of the TargetMachine defaults. The
   // rest of NVPTX isn't friendly to change subtargets per function and
   // so the default TargetMachine will have all of the options.
   const NVPTXTargetMachine &NTM = static_cast<const NVPTXTargetMachine &>(TM);
   const auto* STI = static_cast<const NVPTXSubtarget*>(NTM.getSubtargetImpl());
+  SmallString<128> Str1;
+  raw_svector_ostream OS1(Str1);
 
+  // Emit header before any dwarf directives are emitted below.
+  emitHeader(M, OS1, *STI);
+  OutStreamer->emitRawText(OS1.str());
+}
+
+bool NVPTXAsmPrinter::doInitialization(Module &M) {
   if (M.alias_size()) {
     report_fatal_error("Module has aliases, which NVPTX does not support.");
     return true; // error
   }
   if (!isEmptyXXStructor(M.getNamedGlobal("llvm.global_ctors"))) {
     report_fatal_error(
         "Module has a nontrivial global ctor, which NVPTX does not support.");
     return true;  // error
   }
   if (!isEmptyXXStructor(M.getNamedGlobal("llvm.global_dtors"))) {
     report_fatal_error(
         "Module has a nontrivial global dtor, which NVPTX does not support.");
     return true;  // error
   }
 
-  SmallString<128> Str1;
-  raw_svector_ostream OS1(Str1);
-
   // We need to call the parent's one explicitly.
   bool Result = AsmPrinter::doInitialization(M);
 
-  // Emit header before any dwarf directives are emitted below.
-  emitHeader(M, OS1, *STI);
-  OutStreamer->emitRawText(OS1.str());
-
-  // Emit module-level inline asm if it exists.
-  if (!M.getModuleInlineAsm().empty()) {
-    OutStreamer->AddComment("Start of file scope inline assembly");
-    OutStreamer->AddBlankLine();
-    OutStreamer->emitRawText(StringRef(M.getModuleInlineAsm()));
-    OutStreamer->AddBlankLine();
-    OutStreamer->AddComment("End of file scope inline assembly");
-    OutStreamer->AddBlankLine();
-  }
-
   GlobalsEmitted = false;
 
   return Result;
 }
 
 void NVPTXAsmPrinter::emitGlobals(const Module &M) {
   SmallString<128> Str2;
   raw_svector_ostream OS2(Str2);

llvm/test/CodeGen/NVPTX/module-inline-asm.ll

 ; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s
 
 target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v16:16:16-v32:32:32-v64:64:64-v128:128:128-n16:32:64"
 
+; module asm must come after PTX version/target directives.
+; CHECK-NOT: .global .b32 val;
+
+; CHECK-DAG: .version
+; CHECK-DAG: .target
+
 ; CHECK: .global .b32 val;
 module asm ".global .b32 val;"
 
+; module asm must happen before we emit other things.
+; CHECK-LABEL: .visible .func foo
 define void @foo() {
   ret void
 }
+; Make sure it does not show up anywhere else in the output.
+; CHECK-NOT: .global .b32 val;