[llvm] [MachinePipeliner] Add an abstract layer to manipulate Data Dependenc… (PR #109918)
Ryotaro Kasuga via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 11 05:14:11 PDT 2024
kasuga-fj wrote:
> except that it ignores the fact that antidependences in memory and explicit physical registers also create loop-carried dependences without having any phi nodes
The existing implementation doesn't express loop-carried anti-dependencies in memory and physical registers directly by anti-edges. For memory, such dependencies are detected by calling `isLoopCarriedDep`. I left them as they are, so there should be no problem. Loop-carried anti-dependencies in physical registers don't seem to be taken into account at all. I think this should be fixed, but the problem is independent of this patch. Regarding the code marked "To preserve previous behavior and prevent regression", I think they don't take into account loop-carried dependencies in memory and physical registers. This is based on the fact that if they do consider them, data-dependencies and output-dependencies also should be checked, but they do not.
> I'm particularly concerned about the fact that these antidependences are not reversed before calling createAdjacencyStructure
Now `SwingSchedulerDDG` is used instead of `SwingSchedulerDAG` in `createAdjacencyStructure`, so the detected cycles should remain the same.
> I also note that I would expect some performance changes as a result of this change simply because nodes do get added to queues in a different order
Yes, the iteration order of edges does change. However, I believe that for most functions, their behavior is independent of the order of the edges. `Circuits::circuit` should be affected, but the order is the same as queued, not "within iteration then loop-dependent".
I forgot to mention in the description, but I used llvm-test-suite to verify that there is no change in the compiled result before and after this change. Of course, I don't try every combination of compilation options, but I believe that this patch doesn't change the existing behavior. However, as you said, the existing algorithm is confusing, so there is a great possibility of missing or misunderstanding.
https://github.com/llvm/llvm-project/pull/109918
More information about the llvm-commits
mailing list