This is an archive of the discontinued LLVM Phabricator instance.

Prevent FENTRY_CALL reordering
ClosedPublic

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

Details

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
iii requested review of this revision.Nov 10 2020, 3:53 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

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 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#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 added a comment.Nov 11 2020, 6:44 AM
In D91218#2388586, @iii 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.
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

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 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.
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.
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
This revision was automatically updated to reflect the committed changes.