[PATCH] D57180: [LV] Avoid adding into interleaved group in presence of WAW dependency

Thu Jan 31 23:42:07 PST 2019

hsaito added a comment.

In D57180#1380093 <https://reviews.llvm.org/D57180#1380093>, @dorit wrote:

> In D57180#1376068 <https://reviews.llvm.org/D57180#1376068>, @fhahn wrote:
>
> > I plan on having a look later this week. I am a little worried that the checks in-line here are already quite complex and I would like to have a think if that could be improved in some way.
>
>
> I agree; The algorithm makes sure that we visit everything between B and A, including C, before we visit A; so we have a chance to identify the (potentially) interfering store C before we reach A; This is what allows the algorithm to only compare the pairs (A,B) without having each time to also scan everything in between.
>
> So I think the bug is that when we visited C, and found that it could be inserted into B's group dependence-wise, but wasn't inserted due to other reasons, we should have either:
>
> 1. Invalidated the group (which is over aggressive but better than wrong code)
> 2. Recorded in B's Group the index where C could be inserted, to "burn" that index from allowing some other instruction A to become a group member at that index; so when we reach A we see its spot is taken. (I think this will have the same effect as the proposed patch but without the extra scan.)
> 3. Same as above but instead of bailing out on grouping A with B, make sure that C is alsosunk down with that group (as I think Hideki mentioned in the PR) (maybe a future improvement).

If you don't like the current approach, I agree 2) achieves the same thing, with extra bookkeeping (or extra state in the existing bookkeeping). I think 1) is too conservative. Even if C is next to B, B's group can still be extended to the other direction. 3) should be done separately from the bug fix. Anyway, do we ever deal with so many loads/stores for this efficiency to avoid extra scanning to actually matter? I'm just curious.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57180