[PATCH] D59730: [GVN] teach ConstantFolding correct handling of non-integral addrspace casts

[Jameson Nash via Phabricator via llvm-commits]

vtjnash added inline comments.


================
Comment at: lib/Analysis/ConstantFolding.cpp:101
     return Constant::getAllOnesValue(DestTy);
+  if (DL.isNonIntegralPointerType(C->getType()->getScalarType()) !=
+      DL.isNonIntegralPointerType(DestTy->getScalarType()))
----------------
reames wrote:
> This line is incorrect.  a) It volates the specification of the function (return ConstantExpr::getBitCast(C, DestTy);).  b) for the ASs not to match this would have to be an addrspace cast, not a bitcast.  Which shouldn't reach here.  
Good call. That makes sense. Several cases in the test suite do accidentally reach here. I'll fix the caller to ensure it only passes in valid inputs, and make this an assert here instead.


================
Comment at: lib/Transforms/Utils/VNCoercion.cpp:324
-  // memtransfer is implicitly a raw byte code
-  if (DL.isNonIntegralPointerType(LoadTy->getScalarType()))
-    // TODO: Can allow nullptrs from constant zeros
----------------
reames wrote:
> there's probably something missing here.  You're removing an early exit without introducing a new one.  Given we still need to bail in at least some cases, that's highly suspicious?
It's really hard to write a correct early-exit here, as we need duplicate a substantial chunk of `ConstantFoldLoadFromConstPtr` (as the TODO below hints at) to prove which cases it knows how to handle correctly. This instead now leans on `ConstantFolding` to only create and return valid IR. I think dropping this makes it easier to test also, since we don't need to show that both the early-return here and the fall-through cases would give the right answers.


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

https://reviews.llvm.org/D59730