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

[Jameson Nash via Phabricator via llvm-commits]

vtjnash marked an inline comment as done.
vtjnash added inline comments.


================
Comment at: lib/Transforms/Utils/VNCoercion.cpp:314
+  if (!GV || !GV->isConstant() || !GV->hasDefinitiveInitializer())
     return -1;
 
----------------
vtjnash wrote:
> 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.
Bump. I wanted to confirm you agree now that this part of the change is NFC. I think I've addressed your other comments, so the other commits in the stack should also be ready for re-review also. Thanks!


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

https://reviews.llvm.org/D59729