[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#3661629 <https://reviews.llvm.org/D129161#3661629>, @barannikov88 wrote:

> By the way, there are other issues with the current implementation. From the top of my head:
>
> - getDistance does not look through PHIs, hence the value returned never exceeds 1. This may needlessly increase node functions (ASAP, ALAP etc.).

Interesting point. I wonder if we've made changes here on our end... Not related to this change, but something to note.

> - canReserveResources and reserveResources do not align in the way they work with ProcResourceCount. This may cause a compiler crash.

Our downstream implementation uses a custom implementation for resource allocation, so this hasn't been a major issue.

> - Calculation of latency between two instructions connected through a PHI does not involve scheduling info; the computed latency is always one or zero (don't remember which one exactly).

This was the issue I commented on in both of your comments re: including the back edge of the circuit in the Latency calculations for NodeSet. Our backend 'skips' the PHI and calculates the operand latency across the backedge to the true use.

> - Finally, the schedule the pipeliner carefully crafted is destroyed by the subsequent MI scheduler, which makes it less useful for VLIW in-order targets.

We recently resolved this by implementing a post-expansion insertion of scheduling barriers. Each modulo cycle is considered its own region and is, therefore, not reordered. There's some magic in the post-RA scheduler that undoes this so that we can actually schedule the whole kernel, but I digress.

> There is a room for improvement, but thanks-to-out-of-order-execution few people care.
>
> A workaround for your issue is simple - you just don't call calculateRecMII and estimate MII yourself. You may as well force it to 1 and it will be safe because MII is just a hint for the scheduler. This will affect compilation time, of course.

It's a good workaround for sure, but I believe this is a viable and correct fix to a real bug that may impact non-Hexagon users. I am, however, willing to reconsider if there is real issue with the change.

> Excuse my English

Honestly? I didn't even notice :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129161