[PATCH] D106589: [GlobalOpt] support ConstantExpr use of global address for OptimizeGlobalAddressOfMalloc

[Shimin Cui via Phabricator via llvm-commits]

scui added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:737
+          return false;
+      } else if (auto *CE = dyn_cast<ConstantExpr>(U)) {
+        // Check further the ConstantExpr.
----------------
efriedma wrote:
> I don't think you can just look past all ConstantExprs here?  Maybe `dyn_cast<GEPOperator>(U)`?
You are right. Will change this to only consider bitcast and gep.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:962
   // Loop over all uses of GV, processing them in turn.
-  while (!GV->use_empty()) {
-    if (StoreInst *SI = dyn_cast<StoreInst>(GV->user_back())) {
+  for (auto UI = GV->user_begin(), UE = GV->user_end(); UI != UE;) {
+    // We're likely changing the use list, so use a mutation-safe
----------------
efriedma wrote:
> Not sure why you're changing the pattern used here.  It's a lot easier to show the `while (!GV->use_empty()) {` loop works correctly.
I prefer to use for loops as we need add additional code to handle CE uses, but I can change back to use while loop, and add some code to remove those CE uses to ensure the loop runs correctly.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:1063
+        if (SI->getOperand(0) == V &&
+            SI->getOperand(1)->stripPointerCasts() != GV)
+          return false; // Storing the pointer itself... bad.
----------------
efriedma wrote:
> `VUse.getOperandNo() == 0`?
Thanks for the suggestion. This is the original code as it is, I can change it as you suggested - but looks to me no difference as SI is available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106589