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

[Keno Fischer via Phabricator via llvm-commits]

loladiro added inline comments.


================
Comment at: lib/Transforms/Utils/VNCoercion.cpp:314
+  if (!GV || !GV->isConstant() || !GV->hasDefinitiveInitializer())
     return -1;
 
----------------
vtjnash wrote:
> 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!
I've looked at this and I think I concur with this analysis, so I'll go ahead and land this, since @reames was ok with the rest of it. @reames If we both missed something, please do let me know and I'll be happy to revert or adjust.


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

https://reviews.llvm.org/D59729