[SCCP][PhaseOrdering] Mark Overdefined for loading from null

[StephenFan]

Summary

Currently, SCCPSolver preserves the unknown state of lattice value when
solving a load from null. But a unknown state would be replaced as undef
in the end at method 'getConstantOrNull'.

As a comparison, InstCombine creates a non-terminal unreachable to
inform the SimplifyCFG pass inserting an unreachable instruction.

As test file load-from-null.ll shows, The way that InstCombine does is
better than SCCP. By marking it as overdefined, SCCP would not replace
it as undef, and let InstCombine optimize it to just an unreachable.

[nikic]

What is the original, non-artificial motivation for this?

This will not always be a positive change, because IPSCCP itself may become less effective (e.g. if one of the contributors to a phi is a load from null, we won't be able to ignore that incoming value anymore).

[StephenFan]

In D157408#4572535, @nikic wrote:

What is the original, non-artificial motivation for this?

It was found when I updated the other SCCP patch D153717. So there is no non-artificial motivation for it.

This will not always be a positive change, because IPSCCP itself may become less effective (e.g. if one of the contributors to a phi is a load from null, we won't be able to ignore that incoming value anymore).

Yes. What this patch does makes IPSCCP solve inter-procedure transformation and phis less effective. Can we make willReturn return false for loading null? Then we can preserve these loading from null instructions in many passes and let instcombine and simplifyCFG to transform it to unreachable and delete some dead blocks.

llvm/lib/Transforms/Utils/SCCPSolver.cpp

if (PtrVal.isUnknownOrUndef())
     return; // The pointer is not resolved yet!
 
   ValueLatticeElement &IV = ValueState[&I];
 
   if (SCCPSolver::isConstant(PtrVal)) {
     Constant *Ptr = getConstant(PtrVal, I.getOperand(0)->getType());
 
     // load null is undefined.
-    if (isa<ConstantPointerNull>(Ptr)) {
-      if (NullPointerIsDefined(I.getFunction(), I.getPointerAddressSpace()))
-        return (void)markOverdefined(IV, &I);
-      else
-        return;
-    }
+    if (isa<ConstantPointerNull>(Ptr))
 
     // Transform load (constant global) into the value loaded.
     if (auto *GV = dyn_cast<GlobalVariable>(Ptr)) {
       if (!TrackedGlobals.empty()) {
         // If we are tracking this global, merge in the known value for it.
         auto It = TrackedGlobals.find(GV);
         if (It != TrackedGlobals.end()) {
           mergeInValue(IV, &I, It->second, getMaxWidenStepsOpts());

llvm/test/Transforms/PhaseOrdering/load-from-null.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -O1 -S < %s | FileCheck %s
+; RUN: opt -O2 -S < %s | FileCheck %s
+; RUN: opt -O3 -S < %s | FileCheck %s
+;
+define void @load_invalid_address(ptr %p, i1 %a) {
+; CHECK-LABEL: define void @load_invalid_address
+; CHECK-SAME: (ptr nocapture readnone [[P:%.*]], i1 [[A:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    unreachable
+;
+entry:
+  br i1 %a, label %if.then, label %if.else
+
+if.else:
+  br label %if.then
+
+if.then:                                           ; preds = %lbl, %for.cond1
+  %e.2 = phi ptr [ undef, %entry ], [ null, %if.else ]
+  %0 = load i32, ptr %e.2, align 4
+  store i32 %0, ptr %p
+  ret void
+
+}