[PATCH] D137791: [SDAG] bail out of mergeTruncStores() if there's an unknown store in the chain

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added a comment.



> Yes, this example works fine unless the stores get re-ordered. That was the advantage of the first test draft that included the extra RUN params; it showed that the final asm code was truly a miscompile. I'm ok with any of the test variations so far, so let me know if you want this to change.

Yes, the first one gave a more obvious error, although it was dependent on the scheduler behavior, and therefore not ideal. Since maybe the DAGCombine optimization is not really wrong (if the chains are correctly updated), maybe that wasn't a good idea after all. Maybe your first test was better then, if also the comment said that the store of 0 should follow the two other stores (or a larger one) to @e, or in other words go after all other stores except the one to @d. Or something like that... :-)

>> Could there be a load which is chained after a store, which in turn has a store chained after it? Or any other chained operation besides a store whatever that might be?
>
> I'm not finding a way to show a bug from code like that. If there's a load after one of the stores in our merging set, then it must be independent of any subsequent stores. Therefore, any stores chained in parallel to that load must also be independent. The wide merged store is guaranteed to be chained before the load, so that guarantees that any bits loaded that actually do depend on previous stores are still being stored first.

What if we have four stores to be merged:

N -> St1 -> St2 -> St3 -> Chain

              /
  St -> Ld

Ld may be loading from one or more bytes overlapping St2 and/or St3. St could store to bytes in memory overlapping Ld, St2 and/or St3. Or am I missing something here?

(I guess the confusing thing here to me is that the stores to be merged are chained after one another, while they are actually expected to be non-overlapping by the algorithm...)

Personally I would feel safer if we did not allow any chains at all to get dropped - so we expect all the stores (except N) to only be chained before other stores in the group and nothing else. Ignoring and dropping chains seems best done as a separate optimization (like findBetterChain()), or at least deserves a comment when doing so, I'd say.

It says at the bottom of mergeTruncStores to rely on other DAGCombiner rules to remove the other individual stores. From what I can see in my (original) test case that actually does not happen - one of the i32 stores remain after isel. This seems problematic when considering the final ordering. The NewStore takes the place of the store sequence in the DAG, but if there remains one of them, it is likely then not correctly ordered...? If this is indeed the case that they may remain, then I suppose NewStore should be chained after N instead of Chain so that the ordering is correct. Somehow they should still get removed, though.

>> And what about N, the first node that never gets its uses checked..?
>
> We don't care about values that are chained after `N` - that's the last original store in time in the set. That's because the merged store from this transform happens strictly before `N`. Ie, the chain for the merged store is always updated to the first store in our merged set of stores, and that set has at least 2 values in it.

I can see that now, as the chain of NewStore takes the place of N lastly in the method.


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

https://reviews.llvm.org/D137791