-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AIE2P] Fix modeling of resource conflicts in the scheduling model #313
base: aie-public
Are you sure you want to change the base?
Conversation
d06f251
to
ae601f7
Compare
I added a test for the known conflict that is fixed by this PR, namely, the conflict between |
# | ||
# (c) Copyright 2025 Advanced Micro Devices, Inc. or its affiliates | ||
# RUN: llc -march=aie2p -run-pass=postmisched %s -o - | FileCheck %s | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add a comment about which resource we are checking and in which cycles they are used by the instruction.
Also see AIE/aie2/schedule/resource/*
Is this series of NOPs really caused by a resource conflict, or is there a data dependence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the tests a little bit so that there is no data dependency.
; CHECK-NEXT: $lfl0 = VMOV_alu_mv_mv_x killed $x9 | ||
; CHECK-NEXT: NOP | ||
$p1, $lf0, $r25 = VLDA_FILL_512 $p1, $lf0, $r25 | ||
$lfl0 = VMOV_alu_mv_mv_x $x9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe $lfl1
here to avoid any WAW dependency?
ae601f7
to
ba91f0b
Compare
ba91f0b
to
c458323
Compare
; CHECK-NEXT: NOP | ||
; CHECK-NEXT: $lfl1 = VMOV_alu_mv_mv_x killed $x9 | ||
; CHECK-NEXT: NOP | ||
; CHECK-NEXT: NOP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe add the test in a pre-commit to show that the instruction is moved up by one cycle to avoid the conflict after the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I've run QoR tests with the updated scheduling model and the failed and passed tests remain the same.