[PATCH] D156909: [RISCV] Use NoReg in place of IMPLICIT_DEF for undefined passthru operands

Wed Aug 9 06:22:28 PDT 2023

asb added a comment.

Some high level review comments:

- The intended change and the problem being solved is very clear from the patch description and the bug description - thanks!
- One minor request is that for the code comments, any time you refer to e.g. a "MachineCSE problem" (as in the comment in RISCVRVVInitUndef.cpp) please be more explicit that this is an issue related to the quality of generated code rather than correctness. Just referencing the bug number would be fine. This will be helpful for future code readers if this workaround lives longer than expected.
- The approach implemented here seems sensible + pragmatic to me. Though my confidence of spotting a lurking subtle issue here is low - so it will be great to get input from other reivewers too.
- Given we don't have concrete examples of this causing _big_ problems I think it wouldn't be the end of the world if this wasn't in 17.0.0, but I see the argument for backporting it and overall am probably slightly in favour. If it's going to happen, I think this needs reviewing and landing soon so there's at least a full release candidate cycle including it, giving a chance to back it out if unexpected problems are found.

Summary: I don't spot any issues and this seems sensible to me, but I think other reviewers on the list who work on vector codegen more are better placed to a detailed line-by-line review.

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

https://reviews.llvm.org/D156909