Prevent FENTRY_CALL reordering

[iii]

Summary

FEntryInserter prepends FENTRY_CALL to the first basic block. In case
there are other instructions, PostRA Machine Instruction Scheduler can
move FENTRY_CALL call around. This actually occurs on SystemZ (see the
testcase). This is bad for the following reasons:

• FENTRY_CALL clobbers registers.
• Linux Kernel depends on whatever FENTRY_CALL expands to to be the very first instruction in the function.

Fix by adding isCall attribute to FENTRY_CALL, which prevents reordering
by making it a scheduling boundary for PostRA Machine Instruction
Scheduler.

[jonpa]

An alternative fix would be to modify isSchedulingBoundary() to accept FENTRY_CALL, but this would have questionable semantics.

Did you try to add the isCall flag to the instruction in Target.td? That should make it a scheduling boundary...

[iii]

In D91218#2388311, @jonpa wrote:

An alternative fix would be to modify isSchedulingBoundary() to accept FENTRY_CALL, but this would have questionable semantics.

Did you try to add the isCall flag to the instruction in Target.td? That should make it a scheduling boundary...

I think it would work, but there is the following comment regarding isCall:

/// MachineScheduler does not currently support scheduling across calls. To
/// handle calls, the DAG builder needs to be modified to create register
/// anti/output dependencies on the registers clobbered by the call's regmask
/// operand. In PreRA scheduling, the stack pointer adjustment already prevents
/// scheduling across calls. In PostRA scheduling, we need the isCall to enforce
/// the boundary, but there would be no benefit to postRA scheduling across
/// calls this late anyway.

If I read this correctly, that isCall is a scheduling boundary is an implementation detail that can (even if not likely) change.
Still, adding isCall must be a right thing to do regardless? FENTRY_CALL cannot be anything else.

[iii]

• Add isCall to FENTRY_CALL in Target.td.

[jonpa]

In D91218#2388401, @iii wrote:

• Add isCall to FENTRY_CALL in Target.td.

Do you still need to create the extra basic block?

[iii]

In D91218#2388416, @jonpa wrote:

In D91218#2388401, @iii wrote:

• Add isCall to FENTRY_CALL in Target.td.

Do you still need to create the extra basic block?

I think it's better to keep it in case isCall is changed to not be a scheduling boundary.

Btw, I used the patch a bit more, and realized I broke debuginfo generation, because I did not adjust isCandidateForCallSiteEntry.
So I added a test for this and will send an update once the regtest finishes.

[jonpa]

In D91218#2388586, @iii wrote:

In D91218#2388416, @jonpa wrote:

In D91218#2388401, @iii wrote:

• Add isCall to FENTRY_CALL in Target.td.

Do you still need to create the extra basic block?

I think it's better to keep it in case isCall is changed to not be a scheduling boundary.

I might argue that this seems very unlikely (as far as I know) since it's been that way for years, and just having a separate basic block is also no guarantee -- in the future there might be super-region scheduling, and besides that a block with a single successor could also theoretically be optimized into being merged with that successor. So I think that the only guarantee there is is really a test case which has the call first...

So I would say adding the extra MBB is unnecessary, but someone else may have another opinion..?

[iii]

I guess you are right - cross-bb optimizations can still mess with it they want to. Let's do just isCall here and solve other problems when/if they arise.

[iii]

• Don't put FENTRY_CALL in a separate MBB.

[jonpa]

llvm/test/CodeGen/SystemZ/fentry-debug-info.ll

1–2 ↗

(On Diff #304532)

Maybe comment on what the purpose of this test is?

Why test for the brasl instruction - just to see that it compiles? I am not sure FileCheck is needed if that's the case...

[iii]

• Do not use FileCheck in fentry-debug-info.ll.

[jonpa]

This LGTM now, but let's see what other reviewers think as well...

[iii]

• Fixed a typo in a comment.
• Rebased.

[iii]

Ping.

[jonpa]

ping!

We very much need this in order to build the Linux kernel on SystemZ, so it would be nice if someone could approve this...

[niravd]

LGTM.

llvm/lib/CodeGen/FEntryInserter.cpp

 
 bool FEntryInserter::runOnMachineFunction(MachineFunction &MF) {
   const std::string FEntryName = std::string(
       MF.getFunction().getFnAttribute("fentry-call").getValueAsString());
   if (FEntryName != "true")
     return false;
 
   auto &FirstMBB = *MF.begin();
+  auto *FEntryMBB = MF.CreateMachineBasicBlock();
+  MF.push_front(FEntryMBB);
   auto *TII = MF.getSubtarget().getInstrInfo();
-  BuildMI(FirstMBB, FirstMBB.begin(), DebugLoc(),
+  BuildMI(*FEntryMBB, FEntryMBB->begin(), DebugLoc(),
           TII->get(TargetOpcode::FENTRY_CALL));
+  FEntryMBB->addSuccessor(&FirstMBB);
   return true;
 }
 
 char FEntryInserter::ID = 0;
 char &llvm::FEntryInserterID = FEntryInserter::ID;
 INITIALIZE_PASS(FEntryInserter, "fentry-insert", "Insert fentry calls", false,
                 false)

llvm/test/CodeGen/SystemZ/fentry-no-reorder.ll

+; RUN: llc %s -mtriple=s390x-linux-gnu -mcpu=zEC12 -o - -verify-machineinstrs \
+; RUN:   | FileCheck %s
+
+@PawnTT = dso_local local_unnamed_addr global [2048 x i8] zeroinitializer, align 2
+
+define dso_local void @clear_pawn_tt() local_unnamed_addr #0 {
+entry:
+  call void @llvm.memset.p0i8.i64(i8* nonnull align 2 dereferenceable(2048) getelementptr inbounds ([2048 x i8], [2048 x i8]* @PawnTT, i64 0, i64 0), i8 0, i64 2048, i1 false)
+  ret void
+}
+
+declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg) #1
+
+attributes #0 = { nofree nounwind writeonly "fentry-call"="true" }
+attributes #1 = { argmemonly nofree nosync nounwind willreturn writeonly }
+
+; CHECK: clear_pawn_tt: # @clear_pawn_tt
+; CHECK-NEXT: # %bb.0:
+; CHECK-NEXT: brasl %r0, __fentry__@PLT
+; CHECK-NEXT: # %bb.1: # %entry