[llvm] [AMDGPU] Fix miscompile from stale value cache in LowerBufferFatPointers (PR #216962)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-amdgpu

Author: Arseniy Obolenskiy (aobolensk)

<details>
<summary>Changes</summary>

ConvertedForStore cached a p7->int conversion per Value across the whole function, so a second store of the same fat pointer in a non-dominating block reused an instruction from the first block, breaking dominance

---
Full diff: https://github.com/llvm/llvm-project/pull/216962.diff


2 Files Affected:

- (modified) llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp (+2-12) 
- (modified) llvm/test/CodeGen/AMDGPU/lower-buffer-fat-pointers-p7-in-memory.ll (+38) 


``````````diff
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp b/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp
index 59d5da21fc7d3..2c9c0190e4b76 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp
@@ -435,8 +435,6 @@ class StoreFatPtrsAsIntsAndExpandMemcpyVisitor
     : public InstVisitor<StoreFatPtrsAsIntsAndExpandMemcpyVisitor, bool> {
   BufferFatPtrToIntTypeMap *TypeMap;
 
-  ValueToValueMapTy ConvertedForStore;
-
   IRBuilder<InstSimplifyFolder> IRB;
 
   // Used for memcpy() lowering.
@@ -476,14 +474,8 @@ Value *StoreFatPtrsAsIntsAndExpandMemcpyVisitor::fatPtrsToInts(
     Value *V, Type *From, Type *To, const Twine &Name) {
   if (From == To)
     return V;
-  ValueToValueMapTy::iterator Find = ConvertedForStore.find(V);
-  if (Find != ConvertedForStore.end())
-    return Find->second;
-  if (isBufferFatPtrOrVector(From)) {
-    Value *Cast = IRB.CreatePtrToInt(V, To, Name + ".int");
-    ConvertedForStore[V] = Cast;
-    return Cast;
-  }
+  if (isBufferFatPtrOrVector(From))
+    return IRB.CreatePtrToInt(V, To, Name + ".int");
   if (From->getNumContainedTypes() == 0)
     return V;
   // Structs, arrays, and other compound types.
@@ -506,7 +498,6 @@ Value *StoreFatPtrsAsIntsAndExpandMemcpyVisitor::fatPtrsToInts(
       Ret = IRB.CreateInsertValue(Ret, NewField, Idx);
     }
   }
-  ConvertedForStore[V] = Ret;
   return Ret;
 }
 
@@ -560,7 +551,6 @@ bool StoreFatPtrsAsIntsAndExpandMemcpyVisitor::processFunction(
   for (WeakTrackingVH VH : make_early_inc_range(CanBecomeLoops)) {
     Changed |= visit(cast<Instruction>(VH));
   }
-  ConvertedForStore.clear();
   this->TTI = nullptr;
   this->SE = nullptr;
   return Changed;
diff --git a/llvm/test/CodeGen/AMDGPU/lower-buffer-fat-pointers-p7-in-memory.ll b/llvm/test/CodeGen/AMDGPU/lower-buffer-fat-pointers-p7-in-memory.ll
index 497f99903e266..071666e4a6d75 100644
--- a/llvm/test/CodeGen/AMDGPU/lower-buffer-fat-pointers-p7-in-memory.ll
+++ b/llvm/test/CodeGen/AMDGPU/lower-buffer-fat-pointers-p7-in-memory.ll
@@ -107,3 +107,41 @@ define void @complex_copy(ptr %a, ptr %b) {
   store {[2 x ptr addrspace(7)], i32, ptr addrspace(7)} %x, ptr %b
   ret void
 }
+
+;; The conversion of %p for the store in %then must not be reused by the
+;; store in %else, since neither block dominates the other.
+define void @two_stores_same_value(i1 %cond, ptr addrspace(7) %p, ptr %a, ptr %b) {
+; CHECK-LABEL: define void @two_stores_same_value
+; CHECK-SAME: (i1 [[COND:%.*]], { ptr addrspace(8), i32 } [[P:%.*]], ptr [[A:%.*]], ptr [[B:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[P_RSRC:%.*]] = extractvalue { ptr addrspace(8), i32 } [[P]], 0
+; CHECK-NEXT:    [[P_OFF:%.*]] = extractvalue { ptr addrspace(8), i32 } [[P]], 1
+; CHECK-NEXT:    br i1 [[COND]], label [[THEN:%.*]], label [[ELSE:%.*]]
+; CHECK:       then:
+; CHECK-NEXT:    [[P_INT_RSRC:%.*]] = ptrtoint ptr addrspace(8) [[P_RSRC]] to i160
+; CHECK-NEXT:    [[TMP0:%.*]] = shl nuw i160 [[P_INT_RSRC]], 32
+; CHECK-NEXT:    [[P_INT_OFF:%.*]] = zext i32 [[P_OFF]] to i160
+; CHECK-NEXT:    [[P_INT:%.*]] = or i160 [[TMP0]], [[P_INT_OFF]]
+; CHECK-NEXT:    store i160 [[P_INT]], ptr [[A]], align 32
+; CHECK-NEXT:    br label [[EXIT:%.*]]
+; CHECK:       else:
+; CHECK-NEXT:    [[P_INT1_RSRC:%.*]] = ptrtoint ptr addrspace(8) [[P_RSRC]] to i160
+; CHECK-NEXT:    [[TMP1:%.*]] = shl nuw i160 [[P_INT1_RSRC]], 32
+; CHECK-NEXT:    [[P_INT1_OFF:%.*]] = zext i32 [[P_OFF]] to i160
+; CHECK-NEXT:    [[P_INT1:%.*]] = or i160 [[TMP1]], [[P_INT1_OFF]]
+; CHECK-NEXT:    store i160 [[P_INT1]], ptr [[B]], align 32
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       exit:
+; CHECK-NEXT:    ret void
+;
+entry:
+  br i1 %cond, label %then, label %else
+then:
+  store ptr addrspace(7) %p, ptr %a
+  br label %exit
+else:
+  store ptr addrspace(7) %p, ptr %b
+  br label %exit
+exit:
+  ret void
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/216962