[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

16

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/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp

 //===-- SystemZMCTargetDesc.cpp - SystemZ target descriptions -------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SystemZMCTargetDesc.h"
 #include "SystemZInstPrinter.h"
 #include "SystemZMCAsmInfo.h"
 #include "SystemZTargetStreamer.h"
 #include "TargetInfo/SystemZTargetInfo.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/TargetRegistry.h"
 
 using namespace llvm;
 
 #define GET_INSTRINFO_MC_DESC

 static MCInstPrinter *createSystemZMCInstPrinter(const Triple &T,
                                                  unsigned SyntaxVariant,
                                                  const MCAsmInfo &MAI,
                                                  const MCInstrInfo &MII,
                                                  const MCRegisterInfo &MRI) {
   return new SystemZInstPrinter(MAI, MII, MRI);
 }
 
+void SystemZTargetStreamer::emitConstantPools() {
+  // Emit EXRL target instructions.
+  if (EXRLTargets2Sym.empty())
+    return;
+  // Switch to the .text section.
+  const MCObjectFileInfo &OFI = *Streamer.getContext().getObjectFileInfo();
+  Streamer.SwitchSection(OFI.getTextSection());
+  for (auto &I : EXRLTargets2Sym) {
+    Streamer.emitLabel(I.second);
+    const MCInstSTIPair &MCI_STI = I.first;
+    Streamer.emitInstruction(MCI_STI.first, *MCI_STI.second);
+  }
+  EXRLTargets2Sym.clear();
+}
+
 namespace {
 class SystemZTargetAsmStreamer : public SystemZTargetStreamer {
   formatted_raw_ostream &OS;
 
 public:
   SystemZTargetAsmStreamer(MCStreamer &S, formatted_raw_ostream &OS)
       : SystemZTargetStreamer(S), OS(OS) {}
   void emitMachine(StringRef CPU) override {

llvm/lib/Target/SystemZ/SystemZAsmPrinter.h

 //===-- SystemZAsmPrinter.h - SystemZ LLVM assembly printer ----*- C++ -*--===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_SYSTEMZ_SYSTEMZASMPRINTER_H
 #define LLVM_LIB_TARGET_SYSTEMZ_SYSTEMZASMPRINTER_H
 
 #include "SystemZMCInstLower.h"
 #include "SystemZTargetMachine.h"
+#include "SystemZTargetStreamer.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/StackMaps.h"
 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/Support/Compiler.h"
 
 namespace llvm {
 class MCStreamer;
 class MachineBasicBlock;
 class MachineInstr;
 class Module;
 class raw_ostream;
 
 class LLVM_LIBRARY_VISIBILITY SystemZAsmPrinter : public AsmPrinter {
 private:
   StackMaps SM;
 
-  typedef std::pair<MCInst, const MCSubtargetInfo *> MCInstSTIPair;
-  struct CmpMCInst {
-    bool operator()(const MCInstSTIPair &MCI_STI_A,
-                    const MCInstSTIPair &MCI_STI_B) const {
-      if (MCI_STI_A.second != MCI_STI_B.second)
-        return uintptr_t(MCI_STI_A.second) < uintptr_t(MCI_STI_B.second);
-      const MCInst &A = MCI_STI_A.first;
-      const MCInst &B = MCI_STI_B.first;
-      assert(A.getNumOperands() == B.getNumOperands() &&
-             A.getNumOperands() == 5 && A.getOperand(2).getImm() == 1 &&
-             B.getOperand(2).getImm() == 1 && "Unexpected EXRL target MCInst");
-      if (A.getOpcode() != B.getOpcode())
-        return A.getOpcode() < B.getOpcode();
-      if (A.getOperand(0).getReg() != B.getOperand(0).getReg())
-        return A.getOperand(0).getReg() < B.getOperand(0).getReg();
-      if (A.getOperand(1).getImm() != B.getOperand(1).getImm())
-        return A.getOperand(1).getImm() < B.getOperand(1).getImm();
-      if (A.getOperand(3).getReg() != B.getOperand(3).getReg())
-        return A.getOperand(3).getReg() < B.getOperand(3).getReg();
-      if (A.getOperand(4).getImm() != B.getOperand(4).getImm())
-        return A.getOperand(4).getImm() < B.getOperand(4).getImm();
-      return false;
-    }
-  };
-  typedef std::map<MCInstSTIPair, MCSymbol *, CmpMCInst> EXRLT2SymMap;
-  EXRLT2SymMap EXRLTargets2Sym;
+  SystemZTargetStreamer *getTargetStreamer() {
+    MCTargetStreamer *TS = OutStreamer->getTargetStreamer();
+    assert(TS && "do not have a target streamer");
+    return static_cast<SystemZTargetStreamer *>(TS);
 
 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 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();
     return AsmPrinter::doInitialization(M);
   }
 
 private:
   void LowerFENTRY_CALL(const MachineInstr &MI, SystemZMCInstLower &MCIL);
   void LowerSTACKMAP(const MachineInstr &MI);
   void LowerPATCHPOINT(const MachineInstr &MI, SystemZMCInstLower &Lower);
-  void emitEXRLTargetInstructions();
 };
 } // end namespace llvm
 
 #endif

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

#undef LOWER_HIGH
   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();
 
+    SystemZTargetStreamer *TS = getTargetStreamer();
     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())
+    SystemZTargetStreamer::MCInstSTIPair ET_STI(ET, &MF->getSubtarget());
+    SystemZTargetStreamer::EXRLT2SymMap::iterator I =
+        TS->EXRLTargets2Sym.find(ET_STI);
+    if (I != TS->EXRLTargets2Sym.end())
       DotSym = I->second;
     else
-      EXRLTargets2Sym[ET_STI] = DotSym = OutContext.createTempSymbol();
+      TS->EXRLTargets2Sym[ET_STI] = DotSym = OutContext.createTempSymbol();
     const MCSymbolRefExpr *Dot = MCSymbolRefExpr::create(DotSym, OutContext);
     EmitToStreamer(
         *OutStreamer,
         MCInstBuilder(SystemZ::EXRL).addReg(LenMinus1Reg).addExpr(Dot));
     return;
   }
 
   default:
assert(NumBytes >= EncodedBytes &&
          "Patchpoint can't request size less than the length of a call.");
   assert((NumBytes - EncodedBytes) % 2 == 0 &&
          "Invalid number of NOP bytes requested!");
   while (EncodedBytes < NumBytes)
     EncodedBytes += EmitNop(OutContext, *OutStreamer, NumBytes - EncodedBytes,
                             getSubtargetInfo());
 }
 
-void SystemZAsmPrinter::emitEXRLTargetInstructions() {
-  if (EXRLTargets2Sym.empty())
-    return;
-  // Switch to the .text section.
-  OutStreamer->SwitchSection(getObjFileLowering().getTextSection());
-  for (auto &I : EXRLTargets2Sym) {
-    OutStreamer->emitLabel(I.second);
-    const MCInstSTIPair &MCI_STI = I.first;
-    OutStreamer->emitInstruction(MCI_STI.first, *MCI_STI.second);
-  }
-  EXRLTargets2Sym.clear();
-}
-
 // Convert a SystemZ-specific constant pool modifier into the associated
 // MCSymbolRefExpr variant kind.
 static MCSymbolRefExpr::VariantKind
 getModifierVariantKind(SystemZCP::SystemZCPModifier Modifier) {
   switch (Modifier) {
   case SystemZCP::TLSGD: return MCSymbolRefExpr::VK_TLSGD;
   case SystemZCP::TLSLDM: return MCSymbolRefExpr::VK_TLSLDM;
   case SystemZCP::DTPOFF: return MCSymbolRefExpr::VK_DTPOFF;
bool SystemZAsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
                                               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) {
-  emitEXRLTargetInstructions();
   emitStackMaps(SM);
 }
 
 // Force static initialization.
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeSystemZAsmPrinter() {
   RegisterAsmPrinter<SystemZAsmPrinter> X(getTheSystemZTarget());
 }

llvm/lib/Target/SystemZ/SystemZTargetStreamer.h

 #include "llvm/MC/MCStreamer.h"
 
 namespace llvm {
 
 class SystemZTargetStreamer : public MCTargetStreamer {
 public:
   SystemZTargetStreamer(MCStreamer &S) : MCTargetStreamer(S) {}
 
+  typedef std::pair<MCInst, const MCSubtargetInfo *> MCInstSTIPair;
+  struct CmpMCInst {
+    bool operator()(const MCInstSTIPair &MCI_STI_A,
+                    const MCInstSTIPair &MCI_STI_B) const {
+      if (MCI_STI_A.second != MCI_STI_B.second)
+        return uintptr_t(MCI_STI_A.second) < uintptr_t(MCI_STI_B.second);
+      const MCInst &A = MCI_STI_A.first;
+      const MCInst &B = MCI_STI_B.first;
+      assert(A.getNumOperands() == B.getNumOperands() &&
+             A.getNumOperands() == 5 && A.getOperand(2).getImm() == 1 &&
+             B.getOperand(2).getImm() == 1 && "Unexpected EXRL target MCInst");
+      if (A.getOpcode() != B.getOpcode())
+        return A.getOpcode() < B.getOpcode();
+      if (A.getOperand(0).getReg() != B.getOperand(0).getReg())
+        return A.getOperand(0).getReg() < B.getOperand(0).getReg();
+      if (A.getOperand(1).getImm() != B.getOperand(1).getImm())
+        return A.getOperand(1).getImm() < B.getOperand(1).getImm();
+      if (A.getOperand(3).getReg() != B.getOperand(3).getReg())
+        return A.getOperand(3).getReg() < B.getOperand(3).getReg();
+      if (A.getOperand(4).getImm() != B.getOperand(4).getImm())
+        return A.getOperand(4).getImm() < B.getOperand(4).getImm();
+      return false;
+    }
+  };
+  typedef std::map<MCInstSTIPair, MCSymbol *, CmpMCInst> EXRLT2SymMap;
+  EXRLT2SymMap EXRLTargets2Sym;
+
+  void emitConstantPools() override;
+
   virtual void emitMachine(StringRef CPU) = 0;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_SYSTEMZ_SYSTEMZTARGETSTREAMER_H

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)