-
Notifications
You must be signed in to change notification settings - Fork 13
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
[AIE2] Fix memory access cycle for TM instructions #240
base: aie-public
Are you sure you want to change the base?
Conversation
@@ -34,6 +34,7 @@ body: | | |||
; CHECK-NEXT: VST_SRS_D8_S32_ag_idx_imm killed $p0, 0, killed $cm0, killed $s0, implicit-def $srsrs_of, implicit $crsat, implicit $crrnd, implicit $crsrssign | |||
; CHECK-NEXT: NOP | |||
; 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.
As discussed offline, that extra might hurt QoR. I'd be happy to measure that impact.
If the results aren't good, we could extend
int getMinFirstMemoryCycle() const;
int getMaxFirstMemoryCycle() const;
int getMinLastMemoryCycle() const;
int getMaxLastMemoryCycle() const;
into e.g.
int getMinFirstMemoryCycle(unsigned AddrSpace = 0) const;
int getMaxFirstMemoryCycle(unsigned AddrSpace = 0) const;
int getMinLastMemoryCycle(unsigned AddrSpace = 0) const;
int getMaxLastMemoryCycle(unsigned AddrSpace = 0) const;
Taking the example of AIE2:
getMinFirstMemoryCycle(AIE2::AddressSpaces::none)
should return4
getMinFirstMemoryCycle(AIE2::AddressSpaces::DM)
should return5
getMinFirstMemoryCycle(AIE2::AddressSpaces::TM)
should return4
But then I guess we'll have the same problem because many load/stores will have the default none
address space, even though they actually are in DM
. That would be quite a big change if we also need to infer the AddressSpaces::DM
address space.
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.
Why do you think it will be a big change if we need to infer none
to DM
?
Note : access to tile memory is always annotated at intrinsic level so we will always infer that
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 mean it will be an invasive change, because tests that go from IR to MIR will get different MMOs if the default becomes DM
instead of none
Hi @katerynamuts did you find this info in the architecture file that you had shared with me a month back ? |
|
Exploring the architecture design, it was found that the instructions that store to and load from the tile memory (TM) map access the TM at cycle 4 instead of cycle 5. This PR changes MemoryCycle in AIE2Schedule.td file from 5 to 4 for such instructions.
The fix changes some tests. After checking the changed tests with @martien-de-jong, it became clear that changing the memory cycle changes the first and last memory cycle for the TM instructions from 5 to 4 which are used in
AIEBaseInstrInfo::getConservativeMemoryLatency
used inMaxLatencyFinder
. As a result, in the tests an additional nop is added because of the worst-case analysis used for inter-block scheduling.