[PATCH] D58260: [INLINER] allow inlining of address taken blocks

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/test/Transforms/Inline/blockaddress.ll:60
+define internal void @b() {
+  call void @a(i32 42, i64* bitcast(i8* blockaddress(@b, %1) to i64*))
+  br label %1
----------------
chandlerc wrote:
> You don't actually check that the post inline code is correct though? Could you add precise checks for the relevant *contents* of the function?
> 
> I'd also use CHECK-LABEL based patterns within each function as we do in other (modern) tests.
> 
> I'll actually be surprised if this works. It is possible that the blockaddress constant expression will somehow be rewritten by the code that actually does the inlining, but honestly, I wouldn't expect it to be rewritten naively. I would expect to need to change the inliner itself (as opposed to the cost) to teach it how to rewrite the `blockaddress` constant expressions that are used to reference the cloned basic blocks. I wouldn't expect the current implementation to walk past an operand that is a constant expr, but I've not checked it.
IIUC, `llvm/lib/Transforms/Utils/CloneFunction.cpp#PruningFunctionCloner::CloneBlock()` looks like it is able to handle this correctly:
```
  // It is only legal to clone a function if a block address within that
  // function is never referenced outside of the function.  Given that, we
  // want to map block addresses from the old function to block addresses in
  // the clone. (This is different from the generic ValueMapper
  // implementation, which generates an invalid blockaddress when
  // cloning a function.)
  //
  // Note that we don't need to fix the mapping for unreachable blocks;
  // the default mapping there is safe.
  if (BB->hasAddressTaken()) {
    Constant *OldBBAddr = BlockAddress::get(const_cast<Function*>(OldFunc),
                                            const_cast<BasicBlock*>(BB));
    VMap[OldBBAddr] = BlockAddress::get(NewFunc, NewBB);
  }
  ...
```

I'm currently wrestling with CGSCC, and understanding ref to call edge promotion.  Local patch (based off attached patch in https://bugs.llvm.org/show_bug.cgi?id=40722) is breaking the invariant that "No function transformations should introduce *new* ref edges!" in `llvm/lib/Analysis/CGSCCPassManager.cpp#llvm::updateCGAndAnalysisManagerForFunctionPass()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58260