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

Mon Nov 21 05:47:02 PST 2022

spatel added a comment.

In D137791#3938236 <https://reviews.llvm.org/D137791#3938236>, @jonpa wrote:

> I guess I was just trying to make the test more resilient to any future changes which would require review of the test each time it would have to be re-generated. I also thought it would be more clear to show exactly which optimization we don't want to happen. Thinking more about it, the funny thing is that it actually would be legal to merge the stores as long as the i16 store to @e was chained to follow after. It may be sufficient to have just one of the runlines, and I guess it might not matter much which one...

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.

> 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.

> 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.

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

https://reviews.llvm.org/D137791