[VNCoercion] Disallow coercion between different ni addrspaces

[vchuravy]

Summary

I'm not sure if it would be legal by the IR reference to introduce
an addrspacecast here, since the IR reference is a bit vague on
the exact semantics, but at least for our usage of it (and I
suspect for many other's usage) it is not. For us, addrspacecasts
between non-integral address spaces carry frontend information that the
optimizer cannot deduce afterwards in a generic way (though we
have frontend specific passes in our pipline that do propagate
these). In any case, I'm sure nobody is using it this way at
the moment, since it would have introduced inttoptrs, which
are definitely illegal.

Fixes PR38375

Co-authored-by: Keno Fischer <[e-mail]>

[vchuravy]

• rebase D50010

[merge_guards_bot]

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[vchuravy]

clang format

[merge_guards_bot]

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[vchuravy]

Bump for review. We (Julia) have been carrying this one for a while.

[reames]

LGTM.

As a side note, I don't think you actually need to disallow conversion of the null pointer between two different NI address spaces. If you wanted to improve opt quality, folding your check into the previous one would be beneficial. I'm not going to require that as this a) fixes a correctness bug, and b) has been outstanding for way to long.

[vchuravy]

Thanks! I rebased the PR and landed it.

[t.p.northover]

Also, even on Linux it seems Clang is inclined to link against libgcc rather than libclang by default (at least that's what my Debian one does). When did these functions get into libgcc? I'm worried that a significant proportion of users there will find themselves having to add extra command-line options (whether disabling this or forcing libclang) to produce binaries.

[t.p.northover]

Sorry, ignore that. Wrong review.

llvm/lib/Transforms/Utils/VNCoercion.cpp

bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
   // The store size must be byte-aligned to support future type casts.
   if (llvm::alignTo(StoreSize, 8) != StoreSize)
     return false;
 
   // The store has to be at least as big as the load.
   if (StoreSize < DL.getTypeSizeInBits(LoadTy).getFixedSize())
     return false;
 
+  bool StoredNI = DL.isNonIntegralPointerType(StoredTy->getScalarType());
+  bool LoadNI = DL.isNonIntegralPointerType(LoadTy->getScalarType());
   // Don't coerce non-integral pointers to integers or vice versa.
-  if (DL.isNonIntegralPointerType(StoredVal->getType()->getScalarType()) !=
-      DL.isNonIntegralPointerType(LoadTy->getScalarType())) {
+  if (StoredNI != LoadNI) {
     // As a special case, allow coercion of memset used to initialize
     // an array w/null.  Despite non-integral pointers not generally having a
     // specific bit pattern, we do assume null is zero.
     if (auto *CI = dyn_cast<Constant>(StoredVal))
       return CI->isNullValue();
     return false;
+  } else if (StoredNI && LoadNI &&
+             StoredTy->getPointerAddressSpace() !=
+                 LoadTy->getPointerAddressSpace()) {
+    return false;
   }
 
 
   // The implementation below uses inttoptr for vectors of unequal size; we
   // can't allow this for non integral pointers. We could teach it to extract
   // exact subvectors if desired. 
-  if (DL.isNonIntegralPointerType(StoredTy->getScalarType()) &&
-      StoreSize != DL.getTypeSizeInBits(LoadTy).getFixedSize())
+  if (StoredNI && StoreSize != DL.getTypeSizeInBits(LoadTy).getFixedSize())
     return false;
 
   return true;
 }
 
 template <class T, class HelperClass>
 static T *coerceAvailableValueToLoadTypeHelper(T *StoredVal, Type *LoadedTy,
                                                HelperClass &Helper,

llvm/test/Transforms/GVN/non-integral-pointers.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -gvn -S < %s | FileCheck %s
 
-target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128-ni:4"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128-ni:4:5"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @f0(i1 %alwaysFalse, i64 %val, i64* %loc) {
 ; CHECK-LABEL: @f0(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    store i64 [[VAL:%.*]], i64* [[LOC:%.*]], align 8
 ; CHECK-NEXT:    br i1 [[ALWAYSFALSE:%.*]], label [[NEVERTAKEN:%.*]], label [[ALWAYSTAKEN:%.*]]
 ; CHECK:       neverTaken:
entry:
   %v4 = load <4 x i64 addrspace(4)*>, <4 x i64 addrspace(4)*>* %a, align 32
   %res = load i64 addrspace(4)*, i64 addrspace(4)** %b, align 32
   call void @use.v4(<4 x i64 addrspace(4)*> %v4)
   ret i64 addrspace(4)* %res
 }
 
 declare void @use.v2(<2 x i64 addrspace(4)*>)
 declare void @use.v4(<4 x i64 addrspace(4)*>)
+ define i8 addrspace(5)* @multini(i1 %alwaysFalse, i8 addrspace(4)* %val, i8 addrspace(4)** %loc) {
+ ; CHECK-LABEL: @multini(
+ ; CHECK-NOT: inttoptr
+ ; CHECK-NOT: ptrtoint
+ ; CHECK-NOT: addrspacecast
+  entry:
+   store i8 addrspace(4)* %val, i8 addrspace(4)** %loc
+   br i1 %alwaysFalse, label %neverTaken, label %alwaysTaken
+
+  neverTaken:
+   %loc.bc = bitcast i8 addrspace(4)** %loc to i8 addrspace(5)**
+   %differentas = load i8 addrspace(5)*, i8 addrspace(5)** %loc.bc
+   ret i8 addrspace(5)* %differentas
+
+  alwaysTaken:
+   ret i8 addrspace(5)* null
+ }