This is an archive of the discontinued LLVM Phabricator instance.

[llvm] boilerplate for new callbrprepare codegen IR pass
ClosedPublic

Authored by nickdesaulniers on Dec 12 2022, 10:32 AM.

Details

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 10:32 AM
nickdesaulniers requested review of this revision.Dec 12 2022, 10:32 AM
aeubanks added inline comments.Dec 12 2022, 10:51 AM
llvm/test/CodeGen/PowerPC/O3-pipeline.ll
87 ↗(On Diff #482187)

to prevent this you'll want to make sure the pass preserves the domtree, and add
something like this

but if you're not using the domtree right now, you should actually say setPreservesAll and later change that to only preserve analyses that you're actually preserving when the pass actually does something

llvm/tools/opt/opt.cpp
363–398

sorry for this, I'm trying to make it so this isn't necessary

nickdesaulniers planned changes to this revision.Dec 12 2022, 10:59 AM
nickdesaulniers marked 2 inline comments as done.
  • preserve all analyses, for now

I wouldn't add the pass to the pipeline just yet if it's not even doing anything, but the pass boilerplate looks fine

  • drop pass from pipelines for now
aeubanks accepted this revision.Dec 12 2022, 2:33 PM

lg, assuming we're actually going this direction

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h
717

oh this is the att

This revision is now accepted and ready to land.Dec 12 2022, 2:33 PM
void added a comment.Dec 21 2022, 2:46 PM

Why add a stub for the pass? I think people normally just wait until they have a full implementation ready.

Why add a stub for the pass? I think people normally just wait until they have a full implementation ready.

Commit early commit often. I figured it might be easier to review more bite-sized chunks.

This commit is the stub. The next 4 patches incrementally build off it. The 5th patch then wires in the pass to actually run as part of the default pipelines.

  • rebase, format
void accepted this revision.Jan 18 2023, 3:29 PM

A couple nits, but overall okay.

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h
721

nit: Probably don't need to reformat this comment.

llvm/lib/CodeGen/CallBrPrepare.cpp
10

s/closer//

nickdesaulniers planned changes to this revision.Jan 19 2023, 10:01 AM
jyknight added inline comments.Jan 19 2023, 10:11 AM
llvm/include/llvm/InitializePasses.h
86–87

Odd artifact here.

llvm/lib/CodeGen/CallBrPrepare.cpp
10–18

I think this description needs to be substantially edited for clarity.

A few points on substance:

  • the problem is not unique to physical register outputs, so mentioning that here is just misleading.
  • Worth noting that this is primarily to make things easy for SelectionDAG, and in GlobalISel we may be able to lower directly without the IR pass.
nickdesaulniers marked 2 inline comments as done.
  • fix both nits in comments, as per @void
This revision is now accepted and ready to land.Jan 19 2023, 10:14 AM
nickdesaulniers planned changes to this revision.Jan 19 2023, 10:17 AM
nickdesaulniers added a reviewer: jyknight.
jyknight added inline comments.Jan 19 2023, 11:01 AM
llvm/lib/CodeGen/CallBrPrepare.cpp
10–18

[...] GlobalISel we may be able to lower directly [...]

Or, maybe not...since we probably still need to split critical edges, just not insert a new intrinsic? Dunno.

llvm/lib/CodeGen/CallBrPrepare.cpp
10–18

I don't really have any experience with GlobalISel to provide colorful commentary on what it can or cannot do. For instance, I don't think it can even select inline asm, so callbrprepare is kind of a non-starter until it has at least that support.

What are your thoughts on how I can reword this for clarity? I agree I can drop "physical" from descriptions about "physical registers." Anything else? I'm happy to replace the entire thing outright, if you have suggestions?

nickdesaulniers marked 3 inline comments as done.
  • drop cruft from mis-rebase-merge-conflict-resolution as per @jyknight
  • update comment block as per @jyknight
This revision is now accepted and ready to land.Jan 24 2023, 11:25 AM
llvm/lib/CodeGen/CallBrPrepare.cpp
10–18

Updated to remove mention of specific physical registers in favor of just registers. If you have further thoughts on how best to reword this, please reopen the thread with proposed wording. Marking as done for now.

jyknight added inline comments.Jan 30 2023, 1:57 PM
llvm/lib/CodeGen/CallBrPrepare.cpp
10–21

Suggestion:

This pass lowers callbrs in LLVM IR in order to to assist SelectionDAG's codegen.

In particular, this pass assists in inserting register copies for the output values of a callbr along the edges leading to the indirect target blocks. Though the output SSA value is defined by the callbr instruction itself in the IR representation, the value cannot be copied to the appropriate virtual registers prior to jumping to an indirect label, since the jump occurs within the user-provided assembly blob.

Instead, those copies must occur separately at the beginning of each indirect target. That requires that we create a separate SSA definition in each of them (via llvm.callbr.landingpad), and may require splitting critical edges so we have a location to place the intrinsic. Finally, we remap users of the original callbr output SSA value to instead point to the appropriate llvm.callbr.landingpad value.

Ideally, this could be done inside SelectionDAG, or in the MachineInstruction representation, without the use of an IR-level intrinsic. But, within the current framework, it’s simpler to implement as an IR pass. (If support for callbr in GlobalISel is implemented, it’s worth considering whether this is still required.)

nickdesaulniers marked an inline comment as done.
  • use @jyknight's lucid description for the comment block describing callbr-prepare pass
  • final rebase