[PATCH] D70749: [InstCombine] do not insert nonnull assumption for undef

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/assume-replacing-call.ll:199
+entry:
+  %foo = call i64 @func_test_nonnull(i8* undef) #2
+  br i1 %c, label %ctrue, label %cend
----------------
inglorion wrote:
> efriedma wrote:
> > Isn't this already UB?  From LangRef: "if the parameter [...] is null, the behavior is undefined".
> That's basically how the optimization works. Behavior is undefined if the parameter is null, therefore we can assume it is not null, which unlock optimizations.
> 
> The problem is that we would also do this if the parameter is undef. It may not have been undef in the original program. In the example I was looking at, deadargelim replaced an unused (but declared non-null) parameter with undef, instcombine sank the call and generated a non-null assumption for the parameter (which became call void @llvm.assume(i1 undef)), and simplifycfg then eliminated the entire branch that code was on, leading to different behavior from the original program.
> 
> We could presumably fix this in other ways, but my thinking is that not generating assumptions for undef values is a sensible thing to do.
How was DAE able to replace an argument with undef which was fed to strlen? Is that the real bug?

I agree with Eli, though, with current IR langref rules, it's valid to replace this call with `unreachable` as is. Undef can be anything, we can choose it to be zero, and that would become unreachable. If we actually materialized a zero along this codepath, it would crash strlen if it were executed in the original program.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70749