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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D126574#3549417 <https://reviews.llvm.org/D126574#3549417>, @frasercrmck wrote:

> I suppose the test could be added as a separate file with strict assertions disabled. But I only think that's valuable it if there's a visible codegen bug in that test that we're fixing here. If it's just that the strict assertions trip but the codegen happens to come out okay then adding the test in this patch is best, if you ask me.

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.

> As for the change itself, I'm afraid don't understand the precise issue. In phase 1 and 2 if the current info isn't valid we don't do any compatibility checks at all, do we? We just assign the `NewInfo` to `BBInfo.Change`. So I'm a bit confused where you say "We can use the same compatibility check [...] We in fact did this during the data flow (phase 1 and 2)".  Also I think the use of "can" suggests this is option we have available to us, where in fact we're fixing a bug? So isn't it required?

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.

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.

Honestly, this isn't really an "interesting" bug.  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.


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

https://reviews.llvm.org/D126574