[SystemZ] Recognize mrecord-mcount in backend

[jonpa]

Summary

Emit the mcount_loc_ section with all fentry__calls.

This is done using just raw text like gcc does it (see gcc@06477d3). Is this acceptable?

A check in SystemZDAGToDAGISel::runOnMachineFunction() just like for mnop-mcount to make sure this is only used together with -mfentry.

[uweigand]

This is done using just raw text like gcc does it (see gcc@06477d3). Is this acceptable?

Well, that doesn't really work, since it means the -mrecord-mcount is then simply ignored when using the built-in assembler (i.e. compiling to object code instead of assembler source).

[jonpa]

In D71629#1788666, @uweigand wrote:

This is done using just raw text like gcc does it (see gcc@06477d3). Is this acceptable?

Well, that doesn't really work, since it means the -mrecord-mcount is then simply ignored when using the built-in assembler (i.e. compiling to object code instead of assembler source).

Ah, of course...

[jonpa]

Patch updated to call EmitLabel, SwitchSection() and EmitIntValue() instead of emitting raw text.

I don't know why gcc is emitting a "1:" label... Is it OK to instead emit .Ltmp0:, .Ltmp1: labels (any name) for each fentry?

Not exactly sure exactly what type of section should be created, but calling getELFSection() this way gives the right output, it seems.

Switching back to the previous section causes a ".text" to be printed instead of ".previous".

See mrecord-mcount-01.ll for the output...

[uweigand]

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

561

For consistency, we should really remove the "true" from the "mnop-mcount" attribute as well. (That should be a separate patch.)

571–575

There's no reason to split the code up this way. You might as well do everything in the block above.

574

This is wrong, you need to emit a reference to the "DotSym" above.

I guess GCC uses the GAS unnamed label feature "1b" -- this is not the hex value 0x1b, but a backward reference to the immediately preceding instance of the "1:" label.

llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll

13

So instead of ".quad 27" this should read ".quad .Ltmp0".

[uweigand]

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

575

It's probably better to use a OutStreamer->PushSection() / OutStreamer->PopSection() pair here.

[jonpa]

Patch updated per review.

Beginning to understand how this is supposed to work now, but still not sure why the text assembly looks ok, while I see with objdump --filetype=obj the new section filled with zeros:

llc llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll -mtriple=s390x-linux-gnu -mcpu=z10 -o out.o --filetype=obj; objdump -D -z out.o

0000000000000000 <__mcount_loc>:
0:   00 00 00 00             .long   0x00000000
4:   00 00 00 00             .long   0x00000000
8:   00 00 00 00             .long   0x00000000
c:   00 00 00 00             .long   0x00000000

I was expecting to see something like <test1> and <test2> somwhere there... Will the linker fix this somehow (not sure how it could since it's just zeroes...)?

[jonpa]

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

571–575

Ah, ok.

574

Ah, of course...

[uweigand]

In D71629#1789965, @jonpa wrote:

Patch updated per review.

Beginning to understand how this is supposed to work now, but still not sure why the text assembly looks ok, while I see with objdump --filetype=obj the new section filled with zeros:

llc llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll -mtriple=s390x-linux-gnu -mcpu=z10 -o out.o --filetype=obj; objdump -D -z out.o

0000000000000000 <__mcount_loc>:
0:   00 00 00 00             .long   0x00000000
4:   00 00 00 00             .long   0x00000000
8:   00 00 00 00             .long   0x00000000
c:   00 00 00 00             .long   0x00000000

I was expecting to see something like <test1> and <test2> somwhere there... Will the linker fix this somehow (not sure how it could since it's just zeroes...)?

Yes, this will be done via relocations. You should add -r to the objdump command line, then you'll see them :-)

[uweigand]

Minor cosmetic comment, otherwise this LGTM.

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

562

I think it would be nicer to have the EmitLabel after the PopSection, so that the label is optically close to the instruction it labels. But that's just cosmetics.

[jonpa]

Updated per review: EmitLabel after PopSection.

SystemZISelDAGToDAG::runOnMachineFunction() refactored to combine the two checks for fentry-call.

[uweigand]

LGTM with minor tweak. Thanks!

llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll

33

I believe "mop-mcount" no longer needs "true" here now.

[jonpa]

6be1578

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

OutStreamer.EmitInstruction(MCInstBuilder(SystemZ::BRCLAsm)
                                   .addImm(0).addExpr(Dot), STI);
     OutStreamer.EmitLabel(DotSym);
     return 6;
   }
 }
 
 void SystemZAsmPrinter::LowerFENTRY_CALL(const MachineInstr &MI,
                                          SystemZMCInstLower &Lower) {
+  bool RecordMCount = MF->getFunction().hasFnAttribute("mrecord-mcount");
+  if (RecordMCount)
+    OutStreamer->EmitRawText("1:\n");
+
   MCContext &Ctx = MF->getContext();
   if (MF->getFunction().getFnAttribute("mnop-mcount")
                        .getValueAsString() == "true") {
     EmitNop(Ctx, *OutStreamer, 6, getSubtargetInfo());
-    return;
-  }
-
+  } else {
     MCSymbol *fentry = Ctx.getOrCreateSymbol("__fentry__");
     const MCSymbolRefExpr *Op =
       MCSymbolRefExpr::create(fentry, MCSymbolRefExpr::VK_PLT, Ctx);
     OutStreamer->EmitInstruction(MCInstBuilder(SystemZ::BRASL)
                          .addReg(SystemZ::R0D).addExpr(Op), getSubtargetInfo());
   }
 
+  if (RecordMCount) {
+    OutStreamer->EmitRawText("\t.section __mcount_loc, \"a\",@progbits\n");
+    OutStreamer->EmitRawText("\t.quad 1b\n");
+    OutStreamer->EmitRawText("\t.previous");
+  }
+}
+
 void SystemZAsmPrinter::LowerSTACKMAP(const MachineInstr &MI) {
   const SystemZInstrInfo *TII =
     static_cast<const SystemZInstrInfo *>(MF->getSubtarget().getInstrInfo());
 
   unsigned NumNOPBytes = MI.getOperand(1).getImm();
 
   SM.recordStackMap(MI);
   assert(NumNOPBytes % 2 == 0 && "Invalid number of NOP bytes requested!");

llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp

public:
   SystemZDAGToDAGISel(SystemZTargetMachine &TM, CodeGenOpt::Level OptLevel)
       : SelectionDAGISel(TM, OptLevel) {}
 
   bool runOnMachineFunction(MachineFunction &MF) override {
     const Function &F = MF.getFunction();
     if (F.getFnAttribute("mnop-mcount").getValueAsString() == "true" &&
         F.getFnAttribute("fentry-call").getValueAsString() != "true")
       report_fatal_error("mnop-mcount only supported with fentry-call");
+    if (F.hasFnAttribute("mrecord-mcount") &&
+        F.getFnAttribute("fentry-call").getValueAsString() != "true")
+      report_fatal_error("mrecord-mcount only supported with fentry-call");
 
     Subtarget = &MF.getSubtarget<SystemZSubtarget>();
     return SelectionDAGISel::runOnMachineFunction(MF);
   }
 
   // Override MachineFunctionPass.
   StringRef getPassName() const override {
     return "SystemZ DAG->DAG Pattern Instruction Selection";

llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll

+; RUN: llc %s -mtriple=s390x-linux-gnu -mcpu=z10 -o - -verify-machineinstrs \
+; RUN:   | FileCheck %s
+
+define void @test1() #0 {
+entry:
+  ret void
+
+; CHECK-LABEL: @test1
+; CHECK: 1:
+; CHECK: brasl %r0, __fentry__
+; CHECK: .section __mcount_loc, "a",@progbits
+; CHECK: .quad 1b
+; CHECK: .previous
+; CHECK: br %r14
+}
+
+define void @test2() #1 {
+entry:
+  ret void
+
+; CHECK-LABEL: @test2
+; CHECK: 1:
+; CHECK: brcl 0, .Ltmp0
+; CHECK: .section __mcount_loc, "a",@progbits
+; CHECK: .quad 1b
+; CHECK: .previous
+; CHECK: br %r14
+}
+
+attributes #0 = { "fentry-call"="true" "mrecord-mcount" }
+attributes #1 = { "fentry-call"="true" "mnop-mcount"="true" "mrecord-mcount" }
+

llvm/test/CodeGen/SystemZ/mrecord-mcount-02.ll

+; RUN: not llc %s -mtriple=s390x-linux-gnu -o - 2>&1 | FileCheck %s
+;
+; CHECK: LLVM ERROR: mrecord-mcount only supported with fentry-call
+
+define void @test1() #0 {
+entry:
+  ret void
+}
+
+attributes #0 = { "instrument-function-entry-inlined"="mcount" "mrecord-mcount" }
+