[PATCH] D108699: [LAA] Analyze pointers forked by a select

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D108699#3158014 <https://reviews.llvm.org/D108699#3158014>, @david-arm wrote:

> LGTM!
>
> Hi @huntergr, thanks for making all the changes. I think the patch looks good to
> go for now. It's still worth looking into the MemAccessInfo approach as a follow-up,
> but the patch has been sat in review for long enough (3 months) without any
> fundamental objections so I'd prefer we got something merged now to get the
> functionality defended and unblock future work.

I agree it is not ideal that there's been not much feedback so far, but now that there is some additional feedback/suggestion, I think it would be good to hear additional opinions on the preferred direction here or at least discuss concrete potential alternatives; D114487 <https://reviews.llvm.org/D114487> in particular which should have similar effects, but with less invasive changes, i.e. there's no need to adjust `isNoWrap`, `hasComputableBounds`, `RuntimePointerChecking::insert` or add additional state.

You mention future work blocked by this as a reason for landing this now, however I cannot find any references to patches depending on this work.

> This is currently only enabled
> under an option, so any refactoring for the MemAccessInfo approach can be
> done safely under another patch.

While it is true that it is off by default, it adds substantial complexity to LAA which is already quite complex and the changes are quite spread out. One concern is that it adds an additional way to model 'forked pointers'; we already handle 'forked pointers' via phi nodes (by adding multiple `PointerInfo`s). I am also not sure it should be off by default, once it lands.  IMO better analysis should be the right thing to do and should be quite safe from a performance regression perspective. I don't see a strong reason for not enabling this by default and having wide testing surface potential issues early.


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

https://reviews.llvm.org/D108699