This is an archive of the discontinued LLVM Phabricator instance.

Differential D91218

Prevent FENTRY_CALL reordering
ClosedPublic

Authored by iii on Nov 10 2020, 3:53 PM.

Details

Reviewers
uweigand
jonpa
niravd
hfinkel
craig.topper
Commits
rGd58f112ce038: Prevent FENTRY_CALL reordering
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.

Diff Detail

Event Timeline

iii created this revision.Nov 10 2020, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 3:53 PM
Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript
iii requested review of this revision.Nov 10 2020, 3:53 PM
iii added a subscriber: Andreas-Krebbel.Nov 10 2020, 4:13 PM
Harbormaster completed remote builds in B78379: Diff 304346.Nov 10 2020, 5:25 PM
jonpa added a comment.Nov 11 2020, 4:02 AM

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 added a comment.Nov 11 2020, 4:30 AM
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 added reviewers: niravd, hfinkel, craig.topper.Nov 11 2020, 4:35 AM
iii updated this revision to Diff 304484.Nov 11 2020, 4:53 AM
  • Add isCall to FENTRY_CALL in Target.td.
iii retitled this revision from [SystemZ] Prevent fentry reordering to Prevent FENTRY_CALL reordering.Nov 11 2020, 4:54 AM
iii edited the summary of this revision. (Show Details)
jonpa added a comment.Nov 11 2020, 4:58 AM
In D91218#2388401, @iii wrote:
  • Add isCall to FENTRY_CALL in Target.td.

Do you still need to create the extra basic block?

iii added a comment.Nov 11 2020, 6:25 AM
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.

Harbormaster completed remote builds in B78444: Diff 304484.Nov 11 2020, 6:33 AM
jonpa added a comment.Nov 11 2020, 6:44 AM
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 added a comment.Nov 11 2020, 7:18 AM

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 updated this revision to Diff 304532.Nov 11 2020, 8:52 AM
iii edited the summary of this revision. (Show Details)
  • Don't put FENTRY_CALL in a separate MBB.
Herald added a subscriber: ormris. · View Herald TranscriptNov 11 2020, 8:52 AM
iii edited the summary of this revision. (Show Details)Nov 11 2020, 8:52 AM
jonpa added inline comments.Nov 11 2020, 9:12 AM
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...

Harbormaster completed remote builds in B78469: Diff 304532.Nov 11 2020, 10:24 AM
iii updated this revision to Diff 304573.Nov 11 2020, 10:27 AM
iii edited the summary of this revision. (Show Details)
  • Do not use FileCheck in fentry-debug-info.ll.
Harbormaster completed remote builds in B78492: Diff 304573.Nov 11 2020, 12:55 PM
iii marked an inline comment as done.Nov 11 2020, 3:27 PM
jonpa added a comment.Nov 12 2020, 1:40 AM

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

iii updated this revision to Diff 307062.Nov 23 2020, 6:23 AM
  • Fixed a typo in a comment.
  • Rebased.
Harbormaster completed remote builds in B79789: Diff 307062.Nov 23 2020, 6:58 AM
iii added a comment.Nov 30 2020, 4:39 AM

Ping.

jonpa added a comment.Dec 8 2020, 8:45 AM

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 accepted this revision.Dec 8 2020, 12:58 PM

LGTM.

This revision is now accepted and ready to land.Dec 8 2020, 12:58 PM
This revision was landed with ongoing or failed builds.Dec 8 2020, 4:01 PM
Closed by commit rGd58f112ce038: Prevent FENTRY_CALL reordering (authored by iii). · Explain Why
This revision was automatically updated to reflect the committed changes.
iii added a commit: rGd58f112ce038: Prevent FENTRY_CALL reordering.

Revision Contents

PathSize
llvm/
include/
llvm/
Target/
1 line
lib/
CodeGen/
5 lines
test/
CodeGen/
SystemZ/
20 lines

Diff 304484

llvm/include/llvm/Target/Target.td

Show First 20 Lines • Show All 1,269 Lines • ▼ Show 20 Linesdef PATCHABLE_TYPED_EVENT_CALL : StandardPseudoInstruction {
let mayStore = 1; let mayStore = 1;
let hasSideEffects = 1; let hasSideEffects = 1;
} }
def FENTRY_CALL : StandardPseudoInstruction { def FENTRY_CALL : StandardPseudoInstruction {
let OutOperandList = (outs); let OutOperandList = (outs);
let InOperandList = (ins); let InOperandList = (ins);
let AsmString = "# FEntry call"; let AsmString = "# FEntry call";
let usesCustomInserter = 1; let usesCustomInserter = 1;
let isCall = 1;
let mayLoad = 1; let mayLoad = 1;
let mayStore = 1; let mayStore = 1;
let hasSideEffects = 1; let hasSideEffects = 1;
} }
def ICALL_BRANCH_FUNNEL : StandardPseudoInstruction { def ICALL_BRANCH_FUNNEL : StandardPseudoInstruction {
let OutOperandList = (outs); let OutOperandList = (outs);
let InOperandList = (ins variable_ops); let InOperandList = (ins variable_ops);
let AsmString = ""; let AsmString = "";
▲ Show 20 Lines • Show All 396 Lines • Show Last 20 Lines

llvm/lib/CodeGen/FEntryInserter.cpp

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

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

  • This file was added.
; 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