[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:974
+    // Don't bother with BlockAddress used by CallBr for asm goto.
+    if (isa<BlockAddress>(User))
+      continue;
----------------
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.


================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117509