[llvm] r276364 - GVN-hoist: add missing check for all GEP operands available

[David Majnemer via llvm-commits]

LGTM

On Thu, Jul 21, 2016 at 5:02 PM, Sebastian Pop <sebpop at gmail.com> wrote:

> Fixed like this.  Ok to commit?
>
> diff --git a/llvm/lib/Transforms/Scalar/GVNHoist.cpp
> b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
> index c420089..e7179a3 100644
> --- a/llvm/lib/Transforms/Scalar/GVNHoist.cpp
> +++ b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
> @@ -585,9 +585,14 @@ public:
>        Gep = dyn_cast<GetElementPtrInst>(St->getPointerOperand());
>        Val = dyn_cast<Instruction>(St->getValueOperand());
>        // Check that the stored value is available.
> -      if (Val && !isa<GetElementPtrInst>(Val) &&
> -          !DT->dominates(Val->getParent(), HoistPt))
> -        return false;
> +      if (Val) {
> +        if (isa<GetElementPtrInst>(Val)) {
> +          // Check whether we can compute the GEP at HoistPt.
> +          if (!allOperandsAvailable(Val, HoistPt))
> +            return false;
> +        } else if (!DT->dominates(Val->getParent(), HoistPt))
> +          return false;
> +      }
>      }
>
>      // Check whether we can compute the Gep at HoistPt.
> @@ -613,9 +618,6 @@ public:
>
>      // Also copy Val when it is a GEP.
>      if (Val && isa<GetElementPtrInst>(Val)) {
> -      // Check whether we can compute the GEP at HoistPt.
> -      if (!allOperandsAvailable(Val, HoistPt))
> -        return false;
>        Instruction *ClonedVal = Val->clone();
>        ClonedVal->insertBefore(HoistPt->getTerminator());
>        // Conservatively discard any optimization hints, they may differ
> on the
>
>
>
> On Thu, Jul 21, 2016 at 6:53 PM, David Majnemer
> <david.majnemer at gmail.com> wrote:
> > This will return false after we've already mutated the IR.  We should
> return
> > false earlier.
> >
> > On Thu, Jul 21, 2016 at 4:32 PM, Sebastian Pop via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Author: spop
> >> Date: Thu Jul 21 18:32:39 2016
> >> New Revision: 276364
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=276364&view=rev
> >> Log:
> >> GVN-hoist: add missing check for all GEP operands available
> >>
> >> Modified:
> >>     llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
> >>
> >> Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=276364&r1=276363&r2=276364&view=diff
> >>
> >>
> ==============================================================================
> >> --- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)
> >> +++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Thu Jul 21 18:32:39
> 2016
> >> @@ -613,6 +613,9 @@ public:
> >>
> >>      // Also copy Val when it is a GEP.
> >>      if (Val && isa<GetElementPtrInst>(Val)) {
> >> +      // Check whether we can compute the GEP at HoistPt.
> >> +      if (!allOperandsAvailable(Val, HoistPt))
> >> +        return false;
> >>        Instruction *ClonedVal = Val->clone();
> >>        ClonedVal->insertBefore(HoistPt->getTerminator());
> >>        // Conservatively discard any optimization hints, they may differ
> >> on the
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160721/73f82cb7/attachment.html>