[GVN] Try to be more careful about handling constant zero and SVE

[vtjnash]

Summary

Casting a zero should always be permitted. Make sure the canCoerce check reflects that, and that all users explicitly handle it.

[reames]

I really like what you're trying to do here, but the actual change is problematic. You're mixing non-functional refactoring with semantic change which makes this really hard to understand. Please split this into at least two parts. (I say at least because you appear to have some code motion changes which could be landed trivially without review as well.)

[vtjnash]

Split out part of the changes into two parent commits.

[vtjnash]

Thanks @reames, I appreciate what you've already done here. I had started this a long time ago, where more of it was a functional change, then discovered you had recently already made most of the essential fixes and written the necessary tests. I've changed this into a stack of 3 commits that touch slightly different areas.

[reames]

Waiting for previous patches to land before reviewing.

[reames]

LGTM specifically to the noted parts only. Remainder to be rebased on landed parts and reviewed after a week or so of burn confirms belief named parts are NFC.

lib/Transforms/Utils/VNCoercion.cpp

49 ↗

(On Diff #196653)

Consistency: we should probably be avoiding mmx here based on constant folding logic and langref, but given the original code being restructured doesn't handle this, I'm okay with this being done as followup.

52 ↗

(On Diff #196653)

I'm okay with the above split landing as a NFCI, then reviewing the rest.

330 ↗

(On Diff #196653)

This are grouped with the split above.

[vtjnash]

Split commit into two pieces, each more clearly NFCI

[vtjnash]

rebase

[reames]

llvm/lib/Transforms/Scalar/GVN.cpp

1037

Comment change LGTM if you want to land this separately. (Please do.)

llvm/lib/Transforms/Utils/VNCoercion.cpp

21

This looks on the surface to be a bug that needs fixed in the caller. Maybe there's a subtly I'm missing here though?

33

Given we know this *can't* be a scalable type, the generalization to type size doesn't seem to add any value here?

59

It's not really clear to me what value you get from splitting out the type based vs value based variants. You duplicate code and there doesn't seem to be any benefit (in this patch). I am tempted to reject this change.

445

This looks odd. How can this be NFC if you're actually changing the generated code?

I suspect that your patch description is stale and needs updated. If so, you're also missing tests.

[vtjnash]

I've removed NFCI, since I ended up changing some of the SVE handling when attempting to rebase it

llvm/lib/Transforms/Scalar/GVN.cpp

bool GVN::AnalyzeLoadAvailability(LoadInst *LI, MemDepResult DepInfo,
   // Loading from calloc (which zero initializes memory) -> zero
   if (isCallocLikeFn(DepInst, TLI)) {
     Res = AvailableValue::get(Constant::getNullValue(LI->getType()));
     return true;
   }
 
   if (StoreInst *S = dyn_cast<StoreInst>(DepInst)) {
     // Reject loads and stores that are to the same address but are of
-    // different types if we have to. If the stored value is larger or equal to
+    // different types if we have to. If the stored value is convertable to
     // the loaded value, we can reuse it.
     if (!canCoerceMustAliasedValueToLoad(S->getValueOperand(), LI->getType(),
                                          DL))
       return false;
 
     // Can't forward from non-atomic to atomic without violating memory model.
     if (S->isAtomic() < LI->isAtomic())
       return false;

llvm/lib/Transforms/Utils/VNCoercion.cpp

 #include "llvm/Transforms/Utils/VNCoercion.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "vncoerce"
 
 namespace llvm {
 namespace VNCoercion {
 
 static bool isFirstClassAggregateOrScalableType(Type *Ty) {
   return Ty->isStructTy() || Ty->isArrayTy() || isa<ScalableVectorType>(Ty);
 }
 
-/// Return true if coerceAvailableValueToLoadType will succeed.
-bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
-                                     const DataLayout &DL) {
-  Type *StoredTy = StoredVal->getType();
+bool canCoerceMustAliasedTypeToLoad(Type *StoredTy, Type *LoadTy,
+  // TODO: Caller expects to be able to check the results for aliasing, which
+  // is implemented by checking the sizes. Thus even if we know the types are
+  // identical, the code may later crash.
+  if (isa<ScalableVectorType>(StoredTy) || isa<ScalableVectorType>(LoadTy))
+    return false;
 
   if (StoredTy == LoadTy)
     return true;
 
-  // If the loaded/stored value is a first class array/struct, or scalable type,
-  // don't try to transform them. We need to be able to bitcast to integer.
+  // If the loaded/stored value is a first class array/struct, don't try to
+  // transform them. We need to be able to bitcast to integer.
   if (isFirstClassAggregateOrScalableType(LoadTy) ||
       isFirstClassAggregateOrScalableType(StoredTy))
     return false;
 
-  uint64_t StoreSize = DL.getTypeSizeInBits(StoredTy).getFixedSize();
+  TypeSize StoreSize = DL.getTypeSizeInBits(StoredTy);
 
   // The store size must be byte-aligned to support future type casts.
-  if (llvm::alignTo(StoreSize, 8) != StoreSize)
+  if (!StoreSize.isByteSized())
     return false;
 
   // The store has to be at least as big as the load.
-  if (StoreSize < DL.getTypeSizeInBits(LoadTy).getFixedSize())
+  if (StoreSize.getFixedSize() < DL.getTypeSizeInBits(LoadTy).getFixedSize())
     return false;
 
   // Don't coerce non-integral pointers to integers or vice versa.
-  if (DL.isNonIntegralPointerType(StoredVal->getType()->getScalarType()) !=
-      DL.isNonIntegralPointerType(LoadTy->getScalarType())) {
-    // 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();
+  if (DL.isNonIntegralPointerType(StoredTy->getScalarType()) !=
+      DL.isNonIntegralPointerType(LoadTy->getScalarType()))
     return false;
-  }
-
 
   // The implementation below uses inttoptr for vectors of unequal size; we
-  // can't allow this for non integral pointers.  Wecould teach it to extract
-  // exact subvectors if desired. 
+  // 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())
+      StoreSize.getFixedSize() != DL.getTypeSizeInBits(LoadTy).getFixedSize())
     return false;
-  
   return true;
 }
 
+/// Return true if coerceAvailableValueToLoadType will succeed.
+bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
+                                     const DataLayout &DL) {
+  Type *StoredTy = StoredVal->getType();
+  // If we know the value is available too, then, unlike for
+  // canCoerceMustAliasedTypeToLoad, we expect the caller to not need to
+  // or try to check that the size/offset is inbounds.
+  if (StoredTy == LoadTy)
+    return true;
+  if (auto *CI = dyn_cast<Constant>(StoredVal))
+    // All callers are expected to be able to detect and handle the zeros case.
+    if (CI->isNullValue())
+      if (!isa<ScalableVectorType>(StoredTy) &&
+          !isa<ScalableVectorType>(LoadTy) &&
+          DL.getTypeSizeInBits(StoredTy).getFixedSize() >=
+              DL.getTypeSizeInBits(LoadTy).getFixedSize())
+        return true;
+  return canCoerceMustAliasedTypeToLoad(StoredTy, LoadTy, DL);
+}
+
 template <class T, class HelperClass>
 static T *coerceAvailableValueToLoadTypeHelper(T *StoredVal, Type *LoadedTy,
                                                HelperClass &Helper,
                                                const DataLayout &DL) {
   assert(canCoerceMustAliasedValueToLoad(StoredVal, LoadedTy, DL) &&
          "precondition violation - materialization can't fail");
   if (auto *C = dyn_cast<Constant>(StoredVal))
     StoredVal = ConstantFoldConstant(C, DL);
if (auto *C = dyn_cast<ConstantExpr>(StoredVal))
       StoredVal = ConstantFoldConstant(C, DL);
 
     return StoredVal;
   }
   // If the loaded value is smaller than the available value, then we can
   // extract out a piece from it.  If the available value is too small, then we
   // can't do anything.
   assert(StoredValSize >= LoadedValSize &&
-         "canCoerceMustAliasedValueToLoad fail");
+         "canCoerceMustAliasedTypeToLoad fail");
 
   // Convert source pointers to integers, which can be manipulated.
   if (StoredValTy->isPtrOrPtrVectorTy()) {
     StoredValTy = DL.getIntPtrType(StoredValTy);
     StoredVal = Helper.CreatePtrToInt(StoredVal, StoredValTy);
   }
 
   // Convert vectors and fp to integer, which can be manipulated.
int analyzeLoadFromClobberingMemInst(Type *LoadTy, Value *LoadPtr,
   return -1;
 }
 
 template <class T, class HelperClass>
 static T *getStoreValueForLoadHelper(T *SrcVal, unsigned Offset, Type *LoadTy,
                                      HelperClass &Helper,
                                      const DataLayout &DL) {
   LLVMContext &Ctx = SrcVal->getType()->getContext();
+  if (auto *CI = dyn_cast<Constant>(getUnderlyingObject(SrcVal)))
+    if (CI->isNullValue())
+      return Constant::getNullValue(LoadTy);
 
   // If two pointers are in the same address space, they have the same size,
   // so we don't need to do any truncation, etc. This avoids introducing
   // ptrtoint instructions for pointers that may be non-integral.
   if (SrcVal->getType()->isPointerTy() && LoadTy->isPointerTy() &&
       cast<PointerType>(SrcVal->getType())->getAddressSpace() ==
           cast<PointerType>(LoadTy)->getAddressSpace()) {
     return SrcVal;
T *getMemInstValueForLoadHelper(MemIntrinsic *SrcInst, unsigned Offset,
   uint64_t LoadSize = DL.getTypeSizeInBits(LoadTy).getFixedSize() / 8;
 
   // We know that this method is only called when the mem transfer fully
   // provides the bits for the load.
   if (MemSetInst *MSI = dyn_cast<MemSetInst>(SrcInst)) {
     // memset(P, 'x', 1234) -> splat('x'), even if x is a variable, and
     // independently of what the offset is.
     T *Val = cast<T>(MSI->getValue());
+    if (auto *CI = dyn_cast<Constant>(Val)) {
+      // memset(P, '\0', 1234) -> just directly create the null value for *P
+      // by-passing any later validity checks
+      if (CI->isNullValue())
+        return Constant::getNullValue(LoadTy);
+    }
     if (LoadSize != 1)
       Val =
           Helper.CreateZExtOrBitCast(Val, IntegerType::get(Ctx, LoadSize * 8));
     T *OneElt = Val;
 
     // Splat the value out to the right number of bits.
     for (unsigned NumBytesSet = 1; NumBytesSet != LoadSize;) {
       // If we can double the number of bytes set, do it.