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

Wed Mar 31 07:27:07 PDT 2021

jmorse added a comment.

> Uh, I just figured out how mir tests work.

No worries, there's a first time for everything :),

> I am not exactly sure what you mean here. The fault is caused by debug-info preventing propagation from happening, this patch fixes that problem.

This is true, but I think the patch fixes it by masking the underlying debug-info problem. I've taken a closer look at MCP, I reckon there's a fault in MachineCopyPropagation::BackwardCopyPropagateBlock where it treats all register operands the same, i.e. a "use" by a DBG_VALUE is treated the same as a "use" by any ordinary instruction, causing the debug instruction to prevent an optimisation. 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.

That fits with why the extra optimisation gets rid of the different-code-with-debug problem, optimising through register uses would also optimise through debug uses.

If I'm right about that -- could you add that guard, with a debug specific test (ideally in the llvm/tests/DebugInfo/X86 directory), in a separate patch so that we can consider the debug-info matter separate from the optimisation.

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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98659