[PATCH] D129161: [MachinePipeliner] Consider only direct path successors when calculating circuit latency

[James Nagurne via Phabricator via llvm-commits]

JamesNagurne added a comment.

In D129161#3661350 <https://reviews.llvm.org/D129161#3661350>, @hgreving wrote:

> Are you working on a backend where writing a test for this is feasible, like Hexagon?
>
> I went through your change and your discourse post and to me this is sound. We're using our own downstream implementation, I am not very familiar with the upstream code. Hoping to find one more reviewer for this.

That's always the problem, isn't it! Our downstream target is not public, and I'm not familiar enough with the current users' ISAs and implementation to create a targeted case that would exhibit this problem/solution.

- This is a small, albeit important part of software pipelining, and so pushing a DAG through construction, postprocess, mutation, scheduling, and expansion before anything can be tested in lit sounds difficult
- Being a Machine pass, it'd be nice to have a test for everyone, but this change really is generic. I don't have the expertise to go write for upstream Hexagon, PowerPC, and ARM.

So my thought process leads to a couple possibilities:

- Is there a way to unit-test such things? It would be possible, albeit annoying to include this module and then synthesize the NodeSet to ensure the ii is expected.
- Is there any prior art to using some sort of debug/opt remark output to test things like this? We could emit Res/Rec calculations as part of the pass, and use that to make testing less dependent on code generation

As far as review, I had hoped @dmgreen would additionally check off on it as the team at ARM have been making changes and enabling the pass for some subtargets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129161