[InstCombine] Transform !range metadata to !nonnull when combining loads

[arielb1]

Summary

When combining an integer load with !range metadata that does not include 0 to a pointer load, make sure emit !nonnull metadata on the newly-created pointer load. This prevents the !nonnull metadata from being dropped during a ptrtoint/inttoptr pair.

This fixes PR30597

[hfinkel]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

385

What is the:

N->getNumOperands() == 2

supposed to be checking? The range metadata can have any even number of operands; and we already have an assert for this in llvm::getConstantRangeFromMetadata.

386

Indentation looks odd here. Tabs?

test/Transforms/InstCombine/PR30597.ll

24

This is a nice test case, and I'd keep that, but can you also add a more-direct test case involving the range metadata directly?

[arielb1]

Address nits and add test.

[arielb1]

Fix submission errors.

[hfinkel]

Please upload your patches with full context; see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for instructions.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

1290

Don't commit this white-space change.

test/Transforms/InstCombine/PR30597-2.ll

1 ↗

(On Diff #73383)

I don't see any need to have both of these tests in different files. Please put them in the same file.

test/Transforms/InstCombine/PR30597.ll

2

I understand why you want to run -simplifycfg here to cleanup the output of instcombine for the check, but we generally prefer to use the minimal set of passes. Please just run instcombine and check that the branch condition in the output is the appropriate constant.

[majnemer]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

387

Generally, we just say unsigned.

test/Transforms/InstCombine/PR30597-2.ll

7 ↗

(On Diff #73383)

You should probably pattern match whatever %1 happens to be. We should also match the return.

7 ↗

(On Diff #73383)

What does !0 correspond to? Please match it.

test/Transforms/InstCombine/PR30597.ll

2

I'd go even further and suggest that the test be amended to return an i1.

[arielb1]

address review comments.

[arielb1]

unsigned int -> unsigned.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

385

Cargo cult copypasta. Will remove

386

Sure.

[majnemer]

LGTM

[majnemer]

Committing to https://llvm.org/svn/llvm-project/llvm/trunk ...
A test/Transforms/InstCombine/PR30597.ll
M lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
Committed r283836

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

for (const auto &MDPair : MD) {
     case LLVMContext::MD_dereferenceable:
     case LLVMContext::MD_dereferenceable_or_null:
       // These only directly apply if the new type is also a pointer.
       if (NewTy->isPointerTy())
         NewLoad->setMetadata(ID, N);
       break;
     case LLVMContext::MD_range:
       // FIXME: It would be nice to propagate this in some way, but the type
-      // conversions make it hard. If the new type is a pointer, we could
-      // translate it to !nonnull metadata.
+      // conversions make it hard.
+
+      // If it's a pointer now and the range does not contain 0, make it !nonnull.
+      if (NewTy->isPointerTy()) {
+        unsigned BitWidth = IC.getDataLayout().getTypeSizeInBits(NewTy);
+        if (!getConstantRangeFromMetadata(*N).contains(APInt(BitWidth, 0))) {
+          MDNode *NN = MDNode::get(LI.getContext(), None);
+          NewLoad->setMetadata(LLVMContext::MD_nonnull, NN);
+        }
+      }
       break;
     }
   }
   return NewLoad;
 }
 
 /// \brief Combine a store to a new type.
 ///
Instruction *InstCombiner::visitStoreInst(StoreInst &SI) {
 
   return nullptr;
 }
 
 /// SimplifyStoreAtEndOfBlock - Turn things like:
 ///   if () { *P = v1; } else { *P = v2 }
 /// into a phi node with a store in the successor.
 ///
 /// Simplify things like:
 ///   *P = v1; if () { *P = v2; }
 /// into a phi node with a store in the successor.
 ///
 bool InstCombiner::SimplifyStoreAtEndOfBlock(StoreInst &SI) {
   assert(SI.isUnordered() &&
          "this code has not been auditted for volatile or ordered store case");
 
   BasicBlock *StoreBB = SI.getParent();

test/Transforms/InstCombine/PR30597.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: readonly uwtable
+define i1 @dot_ref_s(i32** noalias nocapture readonly dereferenceable(8)) {
+entry-block:
+  %loadedptr = load i32*, i32** %0, align 8, !nonnull !0
+  %ptrtoint = ptrtoint i32* %loadedptr to i64
+  %inttoptr = inttoptr i64 %ptrtoint to i32*
+  %switchtmp = icmp eq i32* %inttoptr, null
+  ret i1 %switchtmp
+
+; CHECK-LABEL: @dot_ref_s
+; CHECK-NEXT: entry-block:
+; CHECK-NEXT: ret i1 false
+}
+
+; Function Attrs: readonly uwtable
+define i64* @function(i64* noalias nocapture readonly dereferenceable(8)) {
+entry-block:
+  %loaded = load i64, i64* %0, align 8, !range !1
+  %inttoptr = inttoptr i64 %loaded to i64*
+  ret i64* %inttoptr
+; CHECK-LABEL: @function
+; CHECK: %{{.+}} = load i64*, i64** %{{.+}}, align 8, !nonnull
+}
+
+
+!0 = !{}
+!1 = !{i64 1, i64 140737488355327}