[PATCH] D126574: [RISCV] Fix an inconsistency with compatible load/store handling

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

In D126574#3551018 <https://reviews.llvm.org/D126574#3551018>, @reames wrote:

> There is a small codegen diff without small asserts, both versions are "correct".  (i.e. it is not fixing an active miscompile, just the assertion failure.)  I do not plan to precommit unless a reviewer wants it.

No, that's fine. Cheers.

> Let me lay it out for you.
>
> In phase 1, we have no block predecessor state, and thus start with invalid.  We hit the store and use the state of the store.  That's fine as a stating exit state, but may get further refined in phase 2.
>
> In phase 2, we have predecessor states and merge them.  This state may be compatible with the store.  If so, we use the incoming state and do *not* change the state at the store.  Thus, if the store is the only instruction in the block, the output state is the input state.
>
> In phase 3, we have the same predecessor states.  Currently, we don't consider the fact the store may be compatible (i.e. we don't pass MI), and thus don't apply the store compat rule.  As such, we select the state of the store (different from phase 2) and propagate that forward.  We get to the end of block with a different state in this case.

Thanks for the explanation! I think my confusion arose because I saw phase 2 starting with predecessor state (presumably valid) and phase 3 starting with invalid state. I missed the fact that if the phase 3 state is invalid we take the predecessor state before checking compatibility. So yeah I see how the compatibility checks are different between phases 2 and 3.

> In my description, my wording may be a bit sloppy.  The bug is that we not consistent between phase 2 and phase 3.  (phase 1 is somewhat irrelevant since we don't have the predecessor states).  I can adjust the submit comment if you have suggestions on how to clarify.

Maybe something along the lines of "Once we've computed the incoming predecessor state, we //should// use the same compatibility check with knowledge of MI as we did in phase 2 in order to be consistent across all phases." ? It's really just the "can" that throws me off the most.

> We just have two copies of the same code which are supposed to be the same, and they aren't.  We should definitely common up this code, but I wanted to do that post bugfix.

SGTM.

Anyway, this patch LGTM. I also ran our internal testing before and after this patch, and everything passes with this patch applied. I saw lots of assertions before.


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

https://reviews.llvm.org/D126574