[PATCH] D139861: [llvm] boilerplate for new callbrprepare codegen IR pass

Thu Jan 19 14:27:28 PST 2023

nickdesaulniers added inline comments.

================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:9-17
+// This pass lowers callbrs in LLVM IR to something closer that the backend can
+// perform correct instruction selection for.  In particular, callbr may have
+// inline asm with output constraints that require specific physical registers.
+// These physical register will need to be copied back to virtual registers
+// somewhere. If we have critical edges, we may need to split them. Regardless
+// of critical edges, we create distinct new SSA values for each indirect
+// destination of the callbr, then remap users of the direct destinations value
----------------
jyknight wrote:
> void wrote:
> > jyknight wrote:
> > > 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.
> > s/closer//
> > [...] 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.
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139861/new/

https://reviews.llvm.org/D139861