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

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/include/llvm/Target/Target.td

def PATCHABLE_TYPED_EVENT_CALL : StandardPseudoInstruction {
   let mayStore = 1;
   let hasSideEffects = 1;
 }
 def FENTRY_CALL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins);
   let AsmString = "# FEntry call";
   let usesCustomInserter = 1;
+  let isCall = 1;
   let mayLoad = 1;
   let mayStore = 1;
   let hasSideEffects = 1;
 }
 def ICALL_BRANCH_FUNNEL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "";

llvm/lib/CodeGen/MachineInstr.cpp

bool MachineInstr::isCandidateForCallSiteEntry(QueryType Type) const {
   if (!isCall(Type))
     return false;
   switch (getOpcode()) {
   case TargetOpcode::PATCHABLE_EVENT_CALL:
   case TargetOpcode::PATCHABLE_TYPED_EVENT_CALL:
   case TargetOpcode::PATCHPOINT:
   case TargetOpcode::STACKMAP:
   case TargetOpcode::STATEPOINT:
+  case TargetOpcode::FENTRY_CALL:
     return false;
   }
   return true;
 }
 
 bool MachineInstr::shouldUpdateCallSiteInfo() const {
   if (isBundle())
     return isCandidateForCallSiteEntry(MachineInstr::AnyInBundle);

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

+; RUN: llc %s -mtriple=s390x-linux-gnu -o - -verify-machineinstrs \
+; RUN:   | FileCheck %s
+
+define dso_local void @_Z1av() local_unnamed_addr #0 !dbg !7 {
+entry:
+  ret void
+}
+
+attributes #0 = { norecurse nounwind readnone "fentry-call"="true" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 12.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "a.cpp", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!7 = distinct !DISubprogram(name: "a", linkageName: "_Z1av", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null}
+
+; CHECK: brasl %r0, __fentry__@PLT

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