[AsmPrinter, SystemZ] Allow target to add instructions before section is ended.

[jonpa]

Summary

SystemZ adds EXRL target instructions in the end of each file. They need to be added to the text section and just like with constant pools this needs to happen before debug info emission since that may end the text section.

In order to facilitate this a new virtual method emitSectionEndings() has been added that is called at the same point as emitConstantPools().

[aeubanks]

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

[jonpa]

In D109513#2992888, @aeubanks wrote:

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

[uweigand]

In D109513#2994077, @jonpa wrote:

In D109513#2992888, @aeubanks wrote:

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Hmm ... that makes me wonder: can we instead just use the existing emitConstantPools in the SystemZ back end to output the EXRL instructions? They are in effect a form of constants ... That shouldn't require any common code changes then.

[jonpa]

In D109513#2994113, @uweigand wrote:

In D109513#2994077, @jonpa wrote:

In D109513#2992888, @aeubanks wrote:

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Hmm ... that makes me wonder: can we instead just use the existing emitConstantPools in the SystemZ back end to output the EXRL instructions? They are in effect a form of constants ... That shouldn't require any common code changes then.

I read in the original bug report: "...adding the XC instructions in some other text section out of the eyes of debug is not a good option for z/OS (details excluded)". Therefore I thought they better be placed in the text section...

[uweigand]

In D109513#2994352, @jonpa wrote:

In D109513#2994113, @uweigand wrote:

In D109513#2994077, @jonpa wrote:

In D109513#2992888, @aeubanks wrote:

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Hmm ... that makes me wonder: can we instead just use the existing emitConstantPools in the SystemZ back end to output the EXRL instructions? They are in effect a form of constants ... That shouldn't require any common code changes then.

I read in the original bug report: "...adding the XC instructions in some other text section out of the eyes of debug is not a good option for z/OS (details excluded)". Therefore I thought they better be placed in the text section...

Can't a back-end specific emitConstantPools hook just emit those into the .text section? It's called at the same point you proposed new hook would be called, so I thought it should be able to do the same thing ...

[jonpa]

Can't a back-end specific emitConstantPools hook just emit those into the .text section? It's called at the same point you proposed new hook would be called, so I thought it should be able to do the same thing ...

Ah, I guess that could work.

• Personally, I would like to see an 'emitSectionEndings()' in AsmPrinter, since there are already are the similar emitStartOfAsmFile() and emitEndOfAsmFile(). It seems to me good to have a clear point before the sections may be closed (and I made the mistake here myself to emit the EXRL target instructions in emitEndOfAsmFile() in the first place...).

• I also see that emitConstantPools() is called in AsmParser. That should not stop the SystemZ EXRL approach (the EXRLTargets2Sym map should then be empty I think), but it is still somewhat confusing -- I am not sure why that call is needed from there...

• For a target (like SystemZ) that does not need a custom MCTargetStreamer otherwise it is easier to overload an AsmPrinter method directly.

I could try and see how much extra code it would be to implement a MCTargetStreamer in SystemZ (and move the EXRLTargets2Sym map into it probably), but first I would like your opinions that this would actually be better...

[uweigand]

Ah, I see, we've have to implement a MCTargetStreamer first ... In any case, I don't have a strong opinion on this, if the common code change is acceptable, that's of course fine with me. A back-end only solution using a MCTargetStreamer would also be OK.

[jonpa]

Since there does not seem to be any desire for the new hook I proposed and we now have a SystemZTargetStreamer I tried to do this with emitConstantPools() instead.

• Move EXRLTargets2Sym map from SystemZAsmPrinter to SystemZTargetStreamer.
• Replace emitEXRLTargetInstructions() with emitConstantPools().

[uweigand]

Two minor nits inline, otherwise I agree we should go with that approach now.

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp

15 ↗

(On Diff #373422)

Agree with the linter here, please keep the includes sorted alphabetically.

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

553

Maybe move the assert into the getTargetStreamer() routine, just like in the AsmParser? (In fact, do we really need two copies of the getTargetStreamer() routine now?)

[jonpa]

Updated per review.

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

553

I started out with the assert inside getTargetStreamer(), but then got test failures because the TargetStreamer is not necessarily created in some cases for example when output is discarded. This was while I was still doing this in emitEndOfAsmFile(), but I see that now it is actually possible to move the assert back, which I did.

Hmm, maybe we could store a pointer to it when createAsmTargetStreamer() / createObjectTargetStreamer() is called. I think that maybe it's better to use the getters like now when they are used, as this object is not guaranteed to be present...

[uweigand]

LGTM now, thanks!

llvm/include/llvm/CodeGen/AsmPrinter.h

public:
 
   // Targets can, or in the case of EmitInstruction, must implement these to
   // customize output.
 
   /// This virtual method can be overridden by targets that want to emit
   /// something at the start of their file.
   virtual void emitStartOfAsmFile(Module &) {}
 
+  /// Targets can override this to emit something into sections before they
+  /// may be ended. By default, this method calls emitConstantPools().
+  virtual void emitSectionEndings(Module &);
+
   /// This virtual method can be overridden by targets that want to emit
   /// something at the end of their file.
   virtual void emitEndOfAsmFile(Module &) {}
 
   /// Targets can override this to emit stuff before the first basic block in
   /// the function.
   virtual void emitFunctionBodyStart() {}
 

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (!Stubs.empty()) {
         OutStreamer->emitSymbolValue(Stub.second.getPointer(),
                                      DL.getPointerSize());
       }
     }
   }
 
   // This needs to happen before emitting debug information since that can end
   // arbitrary sections.
-  if (auto *TS = OutStreamer->getTargetStreamer())
-    TS->emitConstantPools();
+  emitSectionEndings(M);
 
   // Finalize debug and EH information.
   for (const HandlerInfo &HI : Handlers) {
     NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
                        HI.TimerGroupDescription, TimePassesIsEnabled);
     HI.Handler->endModule();
   }
 
static void emitBasicBlockLoopComments(const MachineBasicBlock &MBB,
   OS << "This ";
   if (Loop->isInnermost())
     OS << "Inner ";
   OS << "Loop Header: Depth=" + Twine(Loop->getLoopDepth()) << '\n';
 
   PrintChildLoopComment(OS, Loop, AP.getFunctionNumber());
 }
 
+/// emitSectionEndings - This is the last point of emission into sections
+/// before they may be ended.
+void AsmPrinter::emitSectionEndings(Module &) {
+  if (auto *TS = OutStreamer->getTargetStreamer())
+    TS->emitConstantPools();
+}
+
 /// emitBasicBlockStart - This method prints the label for the specified
 /// MachineBasicBlock, an alignment (if present) and a comment describing
 /// it if appropriate.
 void AsmPrinter::emitBasicBlockStart(const MachineBasicBlock &MBB) {
   // End the previous funclet and start a new one.
   if (MBB.isEHFuncletEntry()) {
     for (const HandlerInfo &HI : Handlers) {
       HI.Handler->endFunclet();

llvm/lib/Target/SystemZ/SystemZAsmPrinter.h

 public:
   SystemZAsmPrinter(TargetMachine &TM, std::unique_ptr<MCStreamer> Streamer)
       : AsmPrinter(TM, std::move(Streamer)), SM(*this) {}
 
   // Override AsmPrinter.
   StringRef getPassName() const override { return "SystemZ Assembly Printer"; }
   void emitInstruction(const MachineInstr *MI) override;
   void emitMachineConstantPoolValue(MachineConstantPoolValue *MCPV) override;
+  void emitSectionEndings(Module &M) override;
   void emitEndOfAsmFile(Module &M) override;
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        const char *ExtraCode, raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              const char *ExtraCode, raw_ostream &OS) override;
 
   bool doInitialization(Module &M) override {
     SM.reset();

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

case SystemZ::EXRL_Pseudo: {
     unsigned TargetInsOpc = MI->getOperand(0).getImm();
     Register LenMinus1Reg = MI->getOperand(1).getReg();
     Register DestReg = MI->getOperand(2).getReg();
     int64_t DestDisp = MI->getOperand(3).getImm();
     Register SrcReg = MI->getOperand(4).getReg();
     int64_t SrcDisp = MI->getOperand(5).getImm();
 
     MCSymbol *DotSym = nullptr;
     MCInst ET = MCInstBuilder(TargetInsOpc).addReg(DestReg)
       .addImm(DestDisp).addImm(1).addReg(SrcReg).addImm(SrcDisp);
     MCInstSTIPair ET_STI(ET, &MF->getSubtarget());
     EXRLT2SymMap::iterator I = EXRLTargets2Sym.find(ET_STI);
     if (I != EXRLTargets2Sym.end())
       DotSym = I->second;
     else
       EXRLTargets2Sym[ET_STI] = DotSym = OutContext.createTempSymbol();
     const MCSymbolRefExpr *Dot = MCSymbolRefExpr::create(DotSym, OutContext);
bool SystemZAsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
                                               const char *ExtraCode,
                                               raw_ostream &OS) {
   SystemZInstPrinter::printAddress(MAI, MI->getOperand(OpNo).getReg(),
                                    MI->getOperand(OpNo + 1).getImm(),
                                    MI->getOperand(OpNo + 2).getReg(), OS);
   return false;
 }
 
-void SystemZAsmPrinter::emitEndOfAsmFile(Module &M) {
+void SystemZAsmPrinter::emitSectionEndings(Module &M) {
   emitEXRLTargetInstructions();
+}
+
+void SystemZAsmPrinter::emitEndOfAsmFile(Module &M) {
   emitStackMaps(SM);
 }
 
 // Force static initialization.
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeSystemZAsmPrinter() {
   RegisterAsmPrinter<SystemZAsmPrinter> X(getTheSystemZTarget());
 }

llvm/test/CodeGen/SystemZ/memset-06.ll

+; RUN: llc < %s -mtriple=s390x-linux-gnu -generate-arange-section
+;
+; Test that the EXRL target instruction is emitted successfully (before text
+; section is closed).
+
+@a = dso_local global i32* null, align 8, !dbg !0
+@j = dso_local global i32 0, align 4, !dbg !5
+
+define void @fun() !dbg !14 {
+entry:
+  %0 = load i32*, i32** @a, align 8, !dbg !18
+  %1 = bitcast i32* %0 to i8*, !dbg !19
+  %2 = load i32, i32* @j, align 4, !dbg !20
+  %conv = sext i32 %2 to i64, !dbg !20
+  call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 %conv, i1 false), !dbg !19
+  ret void, !dbg !21
+}
+
+declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg) #1
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!9, !10, !11, !12}
+!llvm.ident = !{!13}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "a", scope: !2, file: !3, line: 1, type: !8, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 14.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "file.c", directory: "")
+!4 = !{!0, !5}
+!5 = !DIGlobalVariableExpression(var: !6, expr: !DIExpression())
+!6 = distinct !DIGlobalVariable(name: "j", scope: !2, file: !3, line: 2, type: !7, isLocal: false, isDefinition: true)
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 64)
+!9 = !{i32 7, !"Dwarf Version", i32 4}
+!10 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = !{i32 1, !"wchar_size", i32 4}
+!12 = !{i32 7, !"frame-pointer", i32 2}
+!13 = !{!"clang version 14.0.0"}
+!14 = distinct !DISubprogram(name: "fun", scope: !3, file: !3, line: 3, type: !15, scopeLine: 3, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !17)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null}
+!17 = !{}
+!18 = !DILocation(line: 4, column: 11, scope: !14)
+!19 = !DILocation(line: 4, column: 4, scope: !14)
+!20 = !DILocation(line: 4, column: 15, scope: !14)
+!21 = !DILocation(line: 5, column: 1, scope: !14)