[PATCH] D94180: [SimplifyCFG] Optimize CFG when null is passed to a function with nonnull argument.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6600
+        if (Arg == I &&
+            CB->paramHasAttr(CB->getArgOperandNo(&Arg), Attribute::NonNull))
+          return true;
----------------
xbolva00 wrote:
> jdoerfert wrote:
> > nikic wrote:
> > > Two issues:
> > > 
> > > 1. I believe this is subtle incorrect in conjunction with the GEP rule above. Accessing memory at `gep null, x` is always UB, but passing it to a nonnull argument isn't UB as long as x is not zero.
> > > 
> > > 2. I'm not sure what the current state on this is (@aqjune), but I believe the plan is to make passing null to nonnull argument poison rather than UB. This optimization would only be valid using nonnull + noundef.
> > FWIW, I'm, forever, in favor of making nonnull violation poison not UB.
> 
> 1, but here we only care about foo(null) and not foo(gep null, x).
> See C->isNullValue() check.
> 
> 2, ok, I was wondering about noundef too..
Just checking for GEP here isn't enough, you could also have another bitcast in between for example. The right way to handle it would be to add an extra boolean argument to passingValueIsAlwaysUndefined() that can be changed when looking through a GEP.


================
Comment at: llvm/test/Transforms/SimplifyCFG/UnreachableEliminate.ll:328
+  %phi = phi i8* [ %Y, %entry ], [ null, %if ]
+  %gep = getelementptr inbounds i8, i8* %phi, i64 %I
+  call i8* @foo(i8* %gep)
----------------
With inbounds the transform is actually fine, so we'd want to test without inbounds (or with both).


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

https://reviews.llvm.org/D94180