[PATCH] D59661: [GVN] Try to be more careful about handling constant zero, NFCI

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1037
     // 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.
----------------
Comment change LGTM if you want to land this separately.  (Please do.)


================
Comment at: llvm/lib/Transforms/Utils/VNCoercion.cpp:21
+  // identical, the code may later crash.
+  if (isa<ScalableVectorType>(StoredTy) || isa<ScalableVectorType>(LoadTy))
+    return false;
----------------
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?


================
Comment at: llvm/lib/Transforms/Utils/VNCoercion.cpp:33
 
-  uint64_t StoreSize = DL.getTypeSizeInBits(StoredTy).getFixedSize();
+  TypeSize StoreSize = DL.getTypeSizeInBits(StoredTy);
 
----------------
Given we know this *can't* be a scalable type, the generalization to type size doesn't seem to add any value here?


================
Comment at: llvm/lib/Transforms/Utils/VNCoercion.cpp:59
+/// Return true if coerceAvailableValueToLoadType will succeed.
+bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
+                                     const DataLayout &DL) {
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/VNCoercion.cpp:445
   LLVMContext &Ctx = SrcVal->getType()->getContext();
+  if (auto *CI = dyn_cast<Constant>(getUnderlyingObject(SrcVal)))
+    if (CI->isNullValue())
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59661/new/

https://reviews.llvm.org/D59661