[PATCH] D102711: [GlobalOpt] Handle null check with global pointer variables

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:667
     } else if (const CallInst *CI = dyn_cast<CallInst>(U)) {
       if (CI->getCalledOperand() != V) {
         //cerr << "NONTRAPPING USE: " << *U;
----------------
Only loosely relevant to this patch, but are we really expecting anyone to call the result of malloc?  Or am I misreading this?


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:686
+    } else if (isa<ICmpInst>(U) && isa<ConstantPointerNull>(U->getOperand(1))) {
+      // Ignore icmp X, null
     } else {
----------------
Maybe explicitly note here that the rewriting code has special handling for this case.

Not sure this interacts correctly with AllUsesOfValueWillTrapIfNull recursion.  It looks like the rewriting code can't handle an icmp of a bitcast/GEP/PHI of a load.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:932
+      auto *InitBoolValue = ConstantInt::getBool(
+          GV->getContext(), !GV->hasInitializer() ||
+                                SI->getValueOperand() != GV->getInitializer());
----------------
I think `GV->hasInitializer()` must be true here?

The equality comparison here makes me a little nervous, in case the way we analyze bitcasts changes in the future.  `SI->getValueOperand()->isNullValue()` would probably be more clear anyway.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:954
                                LI->getOrdering(), LI->getSyncScopeID(),
                                LI->isUnordered() ? (Instruction *)ICI : LI);
       InitBoolUsed = true;
----------------
The code that was meant to support the icmp comparison case.

Sinking the load seems wrong; how do we know we aren't sinking it past a store?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102711