Value-number GVNHoist loads by result type as well as pointer address.

[clin1]

Summary

Avoids merge errors when opaque pointers are loaded into different types.

[nikic]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

193

The load type is not necessarily a primitive. I don't think you can get around including the full load type in the key here, otherwise you risk collisions.

llvm/test/Transforms/GVNHoist/opaque-diff-type.ll

2

Use update_test_checks.py please.

[jcranmer-intel]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

192

I'm not sure this is true for pointers in different address spaces.

[clin1]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

192

You're right about that... if we key off the whole type as suggested above, it will fix this problem too.

[clin1]

Widen 2nd element of key to uintptr_t, store entire Type*. Add test for different addrspaces, use update_test_checks.

[jcranmer-intel]

One minor change, but otherwise LGTM

llvm/lib/Transforms/Scalar/GVNHoist.cpp

162

You'll want to transpose the ~ and the (uintptr_t) here.

[hiraditya]

GVN hoist did not have the logic for opaque pointers because opaque pointers were added later IIRC.
Thanks for fixing this!
LGTM.

llvm/lib/Transforms/Scalar/GVNHoist.cpp

 
 // Each element of a hoisting list contains the basic block where to hoist and
 // a list of instructions to be hoisted.
 using HoistingPointInfo = std::pair<BasicBlock *, SmallVecInsn>;
 
 using HoistingPointList = SmallVector<HoistingPointInfo, 4>;
 
 // A map from a pair of VNs to all the instructions with those VNs.
-using VNType = std::pair<unsigned, unsigned>;
+using VNType = std::pair<unsigned, uintptr_t>;
 
 using VNtoInsns = DenseMap<VNType, SmallVector<Instruction *, 4>>;
 
 // CHI keeps information about values flowing out of a basic block.  It is
 // similar to PHI but in the inverse graph, and used for outgoing values on each
 // edge. For conciseness, it is computed only for instructions with multiple
 // occurrences in the CFG because they are the only hoistable candidates.
 //     A (CHI[{V, B, I1}, {V, C, I2}]

 using CHIIt = SmallVectorImpl<CHIArg>::iterator;
 using CHIArgs = iterator_range<CHIIt>;
 using OutValuesType = DenseMap<BasicBlock *, SmallVector<CHIArg, 2>>;
 using InValuesType =
     DenseMap<BasicBlock *, SmallVector<std::pair<VNType, Instruction *>, 2>>;
 
 // An invalid value number Used when inserting a single value number into
 // VNtoInsns.
-enum : unsigned { InvalidVN = ~2U };
+enum : uintptr_t { InvalidVN = ~(uintptr_t)2 };
 
 // Records all scalar instructions candidate for code hoisting.
 class InsnInfo {
   VNtoInsns VNtoScalars;
 
 public:
   // Inserts I and its value number in VNtoScalars.
   void insert(Instruction *I, GVNPass::ValueTable &VN) {

 class LoadInfo {
   VNtoInsns VNtoLoads;
 
 public:
   // Insert Load and the value number of its memory address in VNtoLoads.
   void insert(LoadInst *Load, GVNPass::ValueTable &VN) {
     if (Load->isSimple()) {
       unsigned V = VN.lookupOrAdd(Load->getPointerOperand());
-      VNtoLoads[{V, InvalidVN}].push_back(Load);
+      // With opaque pointers we may have loads from the same pointer with
+      // different result types, which should be disambiguated.
+      VNtoLoads[{V, (uintptr_t)Load->getType()}].push_back(Load);
     }
   }
 
   const VNtoInsns &getVNTable() const { return VNtoLoads; }
 };
 
 // Records all store instructions candidate for code hoisting.
 class StoreInfo {
   VNtoInsns VNtoStores;
 
 public:

llvm/test/Transforms/GVNHoist/opaque-diff-type.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -opaque-pointers -passes="gvn-hoist" -S %s | FileCheck %s
+
+; Checks that gvn-hoist does not try to merge loads of the same source pointer
+; when the results are different types.
+
+define linkonce_odr void @i16i32(ptr %arg) {
+; CHECK-LABEL: @i16i32(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br i1 false, label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[T:%.*]] = load i16, ptr [[ARG:%.*]], align 4
+; CHECK-NEXT:    br label [[BB2]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[T3:%.*]] = load i32, ptr [[ARG]], align 8
+; CHECK-NEXT:    ret void
+;
+bb:
+  br i1 false, label %bb1, label %bb2
+
+bb1:                                              ; preds = %bb
+  %t = load i16, ptr %arg, align 4
+  br label %bb2
+
+bb2:                                              ; preds = %bb1, %bb
+  %t3 = load i32, ptr %arg, align 8
+  ret void
+}
+
+define linkonce_odr void @i32f32(ptr %arg) {
+; CHECK-LABEL: @i32f32(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br i1 false, label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[T:%.*]] = load i32, ptr [[ARG:%.*]], align 4
+; CHECK-NEXT:    br label [[BB2]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[T3:%.*]] = load float, ptr [[ARG]], align 8
+; CHECK-NEXT:    ret void
+;
+bb:
+  br i1 false, label %bb1, label %bb2
+
+bb1:                                              ; preds = %bb
+  %t = load i32, ptr %arg, align 4
+  br label %bb2
+
+bb2:                                              ; preds = %bb1, %bb
+  %t3 = load float, ptr %arg, align 8
+  ret void
+}
+
+define linkonce_odr void @i64ptr(ptr %arg) {
+; CHECK-LABEL: @i64ptr(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br i1 false, label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[T:%.*]] = load i64, ptr [[ARG:%.*]], align 4
+; CHECK-NEXT:    br label [[BB2]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[T3:%.*]] = load ptr, ptr [[ARG]], align 8
+; CHECK-NEXT:    ret void
+;
+bb:
+  br i1 false, label %bb1, label %bb2
+
+bb1:                                              ; preds = %bb
+  %t = load i64, ptr %arg, align 4
+  br label %bb2
+
+bb2:                                              ; preds = %bb1, %bb
+  %t3 = load ptr, ptr %arg, align 8
+  ret void
+}
+
+define linkonce_odr void @ptrptr_diff_aspace(ptr %arg) {
+; CHECK-LABEL: @ptrptr_diff_aspace(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br i1 false, label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[T:%.*]] = load ptr addrspace(4), ptr [[ARG:%.*]], align 4
+; CHECK-NEXT:    br label [[BB2]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[T3:%.*]] = load ptr addrspace(2), ptr [[ARG]], align 8
+; CHECK-NEXT:    ret void
+;
+bb:
+  br i1 false, label %bb1, label %bb2
+
+bb1:                                              ; preds = %bb
+  %t = load ptr addrspace(4), ptr %arg, align 4
+  br label %bb2
+
+bb2:                                              ; preds = %bb1, %bb
+  %t3 = load ptr addrspace(2), ptr %arg, align 8
+  ret void
+}
+
+define linkonce_odr void @ptrptr(ptr %arg) {
+; CHECK-LABEL: @ptrptr(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[T3:%.*]] = load ptr, ptr [[ARG:%.*]], align 4
+; CHECK-NEXT:    br i1 false, label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    br label [[BB2]]
+; CHECK:       bb2:
+; CHECK-NEXT:    ret void
+;
+bb:
+  br i1 false, label %bb1, label %bb2
+
+bb1:                                              ; preds = %bb
+  %t = load ptr, ptr %arg, align 4
+  br label %bb2
+
+bb2:                                              ; preds = %bb1, %bb
+  %t3 = load ptr, ptr %arg, align 8
+  ret void
+}