[PATCH] D117201: [ConstantHoist] Remove check for notional overindexing

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D117201#3241570 <https://reviews.llvm.org/D117201#3241570>, @efriedma wrote:

> There are a few different properties isGEPWithNoNotionalOverIndexing checks for:
>
> 1. We have a GEP constant expression.
> 2. All the operands beyond the first are undef or ConstantInt.
> 3. All the integer indexes are in bounds.
>
> The replacement checks:
>
> 1. We have a GEP constant expression.
> 2. All the operands beyond the first are ConstantInt.
>
> So basically, this is just removing the third condition.
>
> accumulateConstantOffset already checks that all the operands are ConstantInt, so the second condition is redundant; should be fine to just check `isa<GEPOperator>`, I think.

Right, done.

> If we stop checking for over-indexing, do we need to worry about "inbounds" markings?  Before this patch, all the transformed GEPs were effectively inbounds, but with this patch they aren't.

I don't think so. We're just hoisting address arithmetic here, it should not matter whether it is inbounds or not.


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

https://reviews.llvm.org/D117201