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

[Shimin Cui via Phabricator via llvm-commits]

scui 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;
----------------
efriedma wrote:
> Only loosely relevant to this patch, but are we really expecting anyone to call the result of malloc?  Or am I misreading this?
I think we can simply return false when passing to call argument, but I'll leave the code as it is for now. I'm going to extend the code for function call argument passing and function returns later in another patch,


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:686
+    } else if (isa<ICmpInst>(U) && isa<ConstantPointerNull>(U->getOperand(1))) {
+      // Ignore icmp X, null
     } else {
----------------
efriedma wrote:
> 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.
Add restriction for operand 0 as the transformation only handles this kind of code pattern.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:932
+      auto *InitBoolValue = ConstantInt::getBool(
+          GV->getContext(), !GV->hasInitializer() ||
+                                SI->getValueOperand() != GV->getInitializer());
----------------
efriedma wrote:
> 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.
That's right,  GV->hasInitializer() is true here. Modify the code as you suggested. 


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:954
                                LI->getOrdering(), LI->getSyncScopeID(),
                                LI->isUnordered() ? (Instruction *)ICI : LI);
       InitBoolUsed = true;
----------------
efriedma wrote:
> 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?
I agree, the sinking is not safe. Remove the comment, and the code sinking.


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

https://reviews.llvm.org/D102711