[AIE2P] Fix scheduling model to add missing ItinRegClass pairs #521
Conversation
khallouh
requested review from
abhinay-anubola,
abnikant,
andcarminati,
F-Stuckmann,
gbossu,
katerynamuts,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| @@ -64,13 +68,14 @@ def DN_RM_PORT : FuncUnit; | |||
| def DN_WM_PORT : FuncUnit; | |||
| def DONE_FU : FuncUnit; | |||
| def EVENT_UPS_OF : FuncUnit; | |||
| def FIFO_EXTRA_RD_PORT : FuncUnit; | |||
| def FIFO_EXTRA_WR_PORT : FuncUnit; | |||
There was a problem hiding this comment.
The fix in the generator introduces a few more resources with new potential conflicts. The number of resources is no longer an issue with #507 being merged.
| def TM_AD : FuncUnit; | ||
| def UPS__H__A : FuncUnit; | ||
| def UPS__H__K : FuncUnit; |
There was a problem hiding this comment.
UPS__H__K was previously removed manually to respect the maximum number of resources, which was possible because UPS__H__A modelled the exact same conflicts. We get UPS__H__K back now after regenerating the file.
| InstrItinData<II_VMOV_alu_mv_mv_x_eXo_eBMLL, [SimpleCycle<DM_RM_L0_PORT>], [/*dst*/2, /*src*/1], [/*dst*/MV_Bypass, /*src*/NoBypass]>, | ||
| InstrItinData<II_VMOV_alu_mv_mv_x_eXo_eBMLH, [SimpleCycle<DM_RM_L0_PORT>], [/*dst*/2, /*src*/1], [/*dst*/MV_Bypass, /*src*/NoBypass]>, | ||
| InstrItinData<II_VMOV_alu_mv_mv_x_eXo_mStFifoh, [PrefixCycle<FIFO_HL_RA_PORT>, SimpleCycle<ST_FIFO_RD_PORT>], [/*dst*/2, /*src*/1], [/*dst*/MV_Bypass, /*src*/NoBypass]>, |
There was a problem hiding this comment.
This line illustrates the fix. Note the use of MV_Bypass for eXo destination operand
| def FIFO_HL_RA_PORT : FuncUnit; | ||
| def FIFO_HL_WA_PORT : FuncUnit; | ||
| def FIFO_STS__ADVANCE_FIFO_R : FuncUnit; | ||
| def LCKREQ : FuncUnit; | ||
| def LD_FIFO_RA_PORT : FuncUnit; | ||
| def LD_FIFO_WA_PORT : FuncUnit; | ||
| def LXC : FuncUnit; |
There was a problem hiding this comment.
This LXC resource was already gone in the generated file before my fix regarding ItinRegClass pairs. It may be some previous fix in the generator which was not transferred to Peano.
| @@ -1,10 +1,10 @@ | |||
| //===-AIE2PGroupedResourcesGenSchedule.td -----------------------------*- tablegen -*-===// | |||
| //===-AIE2PGenSchedule_groupedResources.td -----------------------------*- tablegen -*-===// | |||
There was a problem hiding this comment.
nit: shouldn't the file name also change?
khallouh
force-pushed
the
hamza.sched.fix
branch
from
7415543 to
f0744b5
Compare
khallouh
force-pushed
the
hamza.sched.fix
branch
from
f0744b5 to
43cd640
Compare
Indeed, I have updated accordingly and now we see somewhat different QoR: |
| ; CHECK-NEXT: mova dc1, #0; movx r1, #4; mov dn1, dn0 | ||
| ; CHECK-NEXT: movs p1, p0; add.nc lc, r1, #-3 | ||
| ; CHECK-NEXT: mova r1, #4; mov dn1, dn0 | ||
| ; CHECK-NEXT: mova dc1, #0; movs p1, p0; add.nc lc, r1, #-3 |
There was a problem hiding this comment.
We now avoid the virtual conflict we had on DC_WM_PORT between mova dc1, #0 and add.nc lc, r1, #-3
| ; CHECK-NEXT: mov p6, sp | ||
| ; CHECK-NEXT: padda [p6], m0; mov p7, sp | ||
| ; CHECK-NEXT: lda m1, [p6, #0]; movxm m0, #-1108 | ||
| ; CHECK-NEXT: mova dc4, #0; mov p6, sp |
There was a problem hiding this comment.
And here we avoid the virtual conflict we had on DC_WM_PORT between mova dc4, #0 and mov p6, sp
We dynamically switch the itinerary data of the instruction based on the register class of the its operands. If for a certain class, no special itinerary is specified, the default instruction itinerary is used, which is more conservative and may lack certain bypasses. This can result in incorrect schedules due to the use of negative latencies.
This PR fixes the scheduling model to add ItinRegClass pairs for register classes that were missing.
The bug was found with VMOV between a fifo store register (e.g.
sfl) and a vector register (e.g.x0) where a bypass should have been specified for the vector register operand as VMOV instructions use the Move Slot Bypass for vector inputs and outputs but not for others (accumulator/fifo).MLLIB results remain unchanged both on a functional and QoR level.
Update: The new itineraries were not used, QoR actually differs for some kernels (See comment below)