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

Fri Nov 18 14:11:13 PST 2022

jonpa added a comment.

> I can add another RUN to check the debug spew from DAGCombiner, but I don't see how that makes the test intent any clearer than the MIR checks + test comment.

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

> I just want to avoid the proven miscompile; that's the most important thing. Optimizations can be added later if that matters.

That's true. I checked on SPEC/SystemZ and this did not change a single file, so bailing seems to be ideal.

> I don't think it is necessary to walk uses of uses on this transform. IIUC, if there's some other problematic store that is included anywhere on this chain, then it must be a use of at least one of the stores to be merged, so we should find it eventually (and give up on the transform).

Yeah, I guess that's true.

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?

And what about N, the first node that never gets its uses checked..?

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

https://reviews.llvm.org/D137791