[PATCH] D48307: [Inliner] Try to inline if some blocks in the callee have address taken, but not used in a meaningful.

[Xin Tong via Phabricator via llvm-commits]

trentxintong added inline comments.


================
Comment at: include/llvm/IR/BasicBlock.h:390
+  /// Return true if the basic block address is taken and used in a meaningful
+  /// way, i.e. non-dead way.
+  bool hasAddressTakenAndUsed();
----------------
davidxl wrote:
> Perhaps change this to: Return true if the basic block address is taken and used by other instructions that can not be proved to be dead.
Will do so.


================
Comment at: include/llvm/IR/BasicBlock.h:391
+  /// way, i.e. non-dead way.
+  bool hasAddressTakenAndUsed();
+
----------------
efriedma wrote:
> Could we just replace hasAddressTaken() with this function?  I don't think anything calling hasAddressTaken() actually cares if there are dead uses.
I feel this is a good way to go. 

However, I feel we still need an API to only check whether getSubclassDataFromValue() is 0 or not. This is needed because BlockAddress::lookup(const BasicBlock *BB) uses it for early exit. And there are other similar uses in the code base. 

I feel we should keep hasAddressTaken() as it is now and make passes use hasAddressTakenAndUsed()  because they do not actually cares if there are dead uses (I suspect most passes if not all). 


================
Comment at: lib/IR/BasicBlock.cpp:446
+  BlockAddress *BA = BlockAddress::get(this);
+  BA->removeDeadConstantUsers();
+  return !BA->use_empty();
----------------
efriedma wrote:
> Please use isConstantUsed instead; mutating the IR in a function named "hasAddressTakenAndUsed" is confusing.
Will do so.


Repository:
  rL LLVM

https://reviews.llvm.org/D48307