[PATCH] D98659: [MachineCopyPropagation] Do more backward copy propagations

Fri Apr 2 14:07:46 PDT 2021

yshui added a comment.

In D98659#2661440 <https://reviews.llvm.org/D98659#2661440>, @jmorse wrote:

> The simplest and easiest fix for that, is to have the function ignore debug-info instructions by adding a "`if (MI.isDebugInstr()) continue;`" statement in the loop before interpreting its register-reads.

This is what @condy did in D98401 <https://reviews.llvm.org/D98401>, but this is incorrect. A debug-info use is still a use, you still need to update them when doing propagation, otherwise the debug-info would be incorrect. And that is what this patch is trying to do.

> That being said: the extra optimisation in this patch is good, as shown by the changes to ll-to-assembly tests, some completely un-necessary copies get eliminated. It's well worth pursuing -- I think more MIR test coverage is needed though (as suggested in  D98659#2633657 <https://reviews.llvm.org/D98659#2633657>), for example:
>
> - A full example without debug-info being involved,
> - Testing that no optimisation happens if the "renamable" flag is absent,
> - Checking that propagation doesn't occur (or occurs correctly modified) when sub-registers are read
>
> Plus the scenarios you list in the comment in the `trackUse` function. The best way to guarantee that those situations are not incorrectly optimised by mistake in the future is by having a test that detects such a scenario. (It's often the case that the tests are a lot more important than the code in the patch itself).

Makes sense, I'll get on with those.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98659