[PATCH] D30642: [ConstantFold] Fix defect in constant folding computation for GEP

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D30642#695286, @javed.absar wrote:

> Hi:
>  Here is a simpler fix for that same defect.
>
> On the point of removing NewIdxs completely, I did give it a try, but then found (a) Idxs is passed as constant reference and so it is not a good idea to modify it;


Ugh.  Unless this code is going to do a thing to them that is bad, that seems silly, but ...
yeah, not gonna make you fix it.

> (b) the algorithm is not really N^2 as  the indexes are re-computed only once.

Once per GEP in the tree, you mean

  // If we did any factoring, start over with the adjusted indices.
  if (!NewIdxs.empty()) {
    for (unsigned i = 0, e = Idxs.size(); i != e; ++i)
      if (!NewIdxs[i]) NewIdxs[i] = cast<Constant>(Idxs[i]);
    return ConstantExpr::getGetElementPtr(PointeeTy, C, NewIdxs, InBounds,
                                          InRangeIndex);

This will call this function, and that call could also adjust and recurse.
all the way up the tree.

So if you constant folded a gep tree like this:

1=GEP 0
2=GEP 1
3=GEP 2
..

Imagine constant folding only fails at the last second for some reason:

calling constant fold on 1 will call it on 0
calling constant fold on 2 will call it on 1 which will call it on 0
calling constant fold on 3 will call it on 2 which will call it on 1 which will call it on 0.

So yes, the function itself is not N^2, but pretty much any normal usage will be.

> Hope this patch with the fix for that specific original problem (stale data reading) is acceptable.
>  Best Regards
> Javed




https://reviews.llvm.org/D30642