[PATCH] D117509: [PartialInline] Bail out on asm-goto/callbr

[Hongtao Yu via Phabricator via llvm-commits]

hoy accepted this revision.
hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/PartialInlining.cpp:974
+    // Don't bother with BlockAddress used by CallBr for asm goto.
+    if (isa<BlockAddress>(User))
+      continue;
----------------
wenlei wrote:
> hoy wrote:
> > Moving the check into `getSupportedCallBase`? The assertion `llvm_unreachable("All uses must be calls");` there seems too strong.
> Thought about that but didn't do that for two reasons: 
> 
> 1. The API assume we would get one call for each User, but BlockAddress is a constant, so it doesn't have 1:1 mapping even for CallBr.
> 2. The contract of assuming valid Call makes sense for other use cases, e.g. getOneCallSiteTo. If we return null for all BlockAddress in getSupportedCallBase, there will be more error handling for getOneCallSiteTo and its caller, which is less clean. 
> 
> So I choose to make changes on the caller side so that the assumption holds for getSupportedCallBase.
Sounds good.


================
Comment at: llvm/lib/Transforms/IPO/PartialInlining.cpp:1420
+    // Don't bother with BlockAddress used by CallBr for asm goto.
+    if (isa<BlockAddress>(User))
+      continue;
----------------
davidxl wrote:
> wenlei wrote:
> > hoy wrote:
> > > davidxl wrote:
> > > > hoy wrote:
> > > > > davidxl wrote:
> > > > > > Is it legal to partial inline in case when the function's block address is taken?
> > > > > I think so. The non-inlined callsites will still call the original version, while the inlined callsites will pull in the new version. The new version has some context-irrelevant code outlined, but both versions should be semantically equal. Please correct me if I'm missing anything?
> > > > what if the labels and references from asm goto are in different regions -- one in inlined region, and the other in the outllined region?
> > > If internal blocks of an inlinee are address taken, the inlining should be disabled during the cost analysis: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/InlineCost.cpp#L2656
> > > 
> > > what if the labels and references from asm goto are in different regions -- one in inlined region, and the other in the outllined region? 
> > 
> > Is this concerning side entrance into the outlined region through asm goto? The outlining for partial inline requires single entry single exit region, so asm goto should not introduce side entrance. 
> > 
> > > If internal blocks of an inlinee are address taken, the inlining should be disabled during the cost analysis: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/InlineCost.cpp#L2656
> > 
> > The code you linked still allow callbr/asm-goto, it disables inlining if address taken blocks are used outside of asm-goto.
> In general, the legality check should be done earlier otherwise we may end up with situation where function outlining happens but partial inlining is disabled -- resulting in less efficient code.  
> 
> However as Wenlei mentioned, cross region jumps may not be an issue.
> The code you linked still allow callbr/asm-goto, it disables inlining if address taken blocks are used outside of asm-goto.

Is asm-goto equivalent to callbr? I thought callbr can only deal with function entry but not internal blocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117509