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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:397
+  /// parameter to a call as potentially escaping.
+  bool addressPotentiallyEscapesFunction();
+
----------------
The issue is not escape, but capture if I understand correctly?

We already have an analysis that answers this question generically: `PointerMayBeCaptured`. I don't think you need to add a new API.


================
Comment at: llvm/test/Transforms/Inline/blockaddress.ll:7
 ; which could be unsafe
-; CHECK: store i8* blockaddress(@doit, %here), i8** %pptr, align 8
+; CHECK-XXX: store i8* blockaddress(@doit, %here), i8** %pptr, align 8
 
----------------
?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58260