[AIE2] Fix VMOV instruction itinerary #239
Conversation
krishnamtibrewala
requested review from
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
khallouh,
konstantinschwarz,
martien-de-jong,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| ItinRegClassPair<II_VMOV_W_WML_WMH,[OperandRegClass<0, eWL>, OperandRegClass<1, eWH>]>] in { | ||
| ItinRegClassPair<II_VMOV_W_WML_WMH,[OperandRegClass<0, eWL>, OperandRegClass<1, eWH>]>, | ||
| ItinRegClassPair<II_VMOV_W_WML_Q, [OperandRegClass<0, eWL>, OperandRegClass<1, mQQm>]>, | ||
| ItinRegClassPair<II_VMOV_W_WMH_Q, [OperandRegClass<0, eWH>, OperandRegClass<1, mQQm>]>] in { | ||
| def VMOV_mv_w : AIE2_mv_w_inst_mv< (outs OP_mMvAMWQDst:$dst), (ins OP_mMvAMWQSrc:$src), |
There was a problem hiding this comment.
Could you think of an easy way to check that the II_VMOV_W itinerary is never used by the postmisched? Doing so can result in problems like the one you describe. Maybe in getSchedClass we should assert if we fallback to the default itinerary because none of the operands matched?
There was a problem hiding this comment.
is that specific for II_VMOV_W or for all the reg-based itinerary ?
If its specific to II_VMOV_W itinerary I can update the tablegen flow, where in an extra field if marked with an instruction will introduce an assert. Also doing this specifically for postmisched will be tricky since we do not have any info in the newly autogenerated getSchedClass function.
Doing it for all is easy but will require adding lot of itinerary manually for MOV instructions since we use conservative itinerary in cases when we are writing to single register like SP, LR , LE .... and so on.
There was a problem hiding this comment.
I think we should aim to have "fully-covering" reg-based itineraries actually. But yes, I don't know how much work that might be to actually make all of our itineraries complete. I think it's fine for now.
krishnamtibrewala
force-pushed
the
aie2-fix-FC_aie2_bf16
branch
2 times, most recently
from
1023fdc to
c94d501
Compare
| ItinRegClassPair<II_VMOV_W_WML_WMH,[OperandRegClass<0, eWL>, OperandRegClass<1, eWH>]>] in { | ||
| ItinRegClassPair<II_VMOV_W_WML_WMH,[OperandRegClass<0, eWL>, OperandRegClass<1, eWH>]>, | ||
| ItinRegClassPair<II_VMOV_W_WML_Q, [OperandRegClass<0, eWL>, OperandRegClass<1, mQQm>]>, | ||
| ItinRegClassPair<II_VMOV_W_WMH_Q, [OperandRegClass<0, eWH>, OperandRegClass<1, mQQm>]>] in { |
There was a problem hiding this comment.
What about VMOV from Q to WL? I assume the output can use the MOV bypass then. Do we need new II_VMOV_W_Q_WML and II_VMOV_W_Q_WMH itineraries?
There was a problem hiding this comment.
As per the ISA, Move slot bypass are only used when we are reading from either a X or WL register.
There was a problem hiding this comment.
What I meant is that
Below, the bypass should be taken
$wl1 = VMOV_mv_w $q0
$q1= VMOV_mv_w $wl1
But not here:
$hl1 = VMOV_mv_w $q0
$q1= VMOV_mv_w $wh1
However, I'm not sure that will be the case due to how we define the itineraries. In any case, it is worth testing :)
There was a problem hiding this comment.
Fixed in the second commit
gbossu
force-pushed
the
aie2-fix-FC_aie2_bf16
branch
from
c94d501 to
e1a7618
Compare
Changes done in this PR were identified by @konstantinschwarz for fixing the DIFF issue in FullyConnect_aie2_bf16
ISSUE : In a specific case the compiler was assuming that VMOV has a latency of 2, i.e. it can still use the old value of $wlX when written from $qX reg, but since the DST reg was of $wlX type which uses the bypass path and overwrites the value one cycle earlier, leading to wrong value being read.