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

Fraser Cormack via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 1 00:39:33 PDT 2022


frasercrmck added a comment.

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.

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?


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

https://reviews.llvm.org/D126574



More information about the llvm-commits mailing list