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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 18 13:41:13 PDT 2021


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



More information about the llvm-commits mailing list