[PATCH] D59729: [GVN] non-functional code movement

[Jameson Nash via Phabricator via llvm-commits]

vtjnash marked 2 inline comments as done.
vtjnash added inline comments.


================
Comment at: lib/Transforms/Utils/VNCoercion.cpp:314
+  if (!GV || !GV->isConstant() || !GV->hasDefinitiveInitializer())
     return -1;
 
----------------
reames wrote:
> Remove this line, not NFC
As far as I could tell, all later code paths are guarded by the full condition that I'm now also using here:

https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L538
https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L669
https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L620
https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L606

I thought it seemed odd to check only part of the condition early, while deferring the full check until later.


================
Comment at: lib/Transforms/Utils/VNCoercion.cpp:331
   // offset applied as appropriate.
-  Src =
-      ConstantExpr::getBitCast(Src, Type::getInt8PtrTy(Src->getContext(), AS));
-  Constant *OffsetCst =
-      ConstantInt::get(Type::getInt64Ty(Src->getContext()), (unsigned)Offset);
-  Src = ConstantExpr::getGetElementPtr(Type::getInt8Ty(Src->getContext()), Src,
-                                       OffsetCst);
+  if (Offset) {
+    Src =
----------------
OTOH, this might not be strictly NFC, since, while not a semantic difference in IR, I thought it seemed like `ConstantFoldLoadFromConstPtr` may sometimes have difficultly looking through this BitCast.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59729