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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
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;
----------------
hoy wrote:
> 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.
>> 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. 

The only use of callbr is for asm-goto, so yes in practice they're equivalent. 

But the code you linked only bails out when the use of block address is *not* callbr. So it doesn't block callbr, which means this statement below isn't true.

> If internal blocks of an inlinee are address taken, the inlining should be disabled during the cost analysis

```
if (BB->hasAddressTaken())
      for (User *U : BlockAddress::get(&*BB)->users())
        if (!isa<CallBrInst>(*U))
          return InlineResult::failure("blockaddress used outside of callbr");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117509