[PATCH] D132205: [llvm-tblgen] CodeGenSchedModels::hasReadOfWrite gets wrong predication result

[Zixuan Wu via Phabricator via llvm-commits]

zixuan-wu added a comment.

In D132205#3780383 <https://reviews.llvm.org/D132205#3780383>, @andreadb wrote:

> The chage looks good. However, the test must be improved.
>
> Your test only checks the content of the write latency table. However, that is not enough to prove that this bug is fixed; you also need to show that the read advance entry for Read_D is associated with Write_B.
>
> I suggest to modify the InstRW for Inst_C as follows:
>
>   def : InstRW<[Write_C, Read_D], (instrs Inst_C)>;
>
> That way, Read_D will be associated to the first read operand of Inst_C.
>
> Since Read_D is now explicitly used, a new `MCReadAvanceEntry` is added to table `MyTargetReadAdvanceTable` (see entry #1 below).
>
>   // {UseIdx, WriteResourceID, Cycles}
>   extern const llvm::MCReadAdvanceEntry MyTargetReadAdvanceTable[] = {
>     {0,  0,  0}, // Invalid
>     {0,  2,  1} // #1
>   }; // MyTargetReadAdvanceTable
>
> In your test, you need to CHECK the presence of that {0, 2, 1} entry. That's the only way to know that there is a (at least one) SchedRead associated to Write_B (WriteResourceID = 2).
> You should also check that Inst_C now uses that ReadAdvance. That's how we know that Read_D is effectively associated with Write_B.
>
> In the .inc output, you should be able to check the last line of the table below:
>
>   // {Name, NumMicroOps, BeginGroup, EndGroup, RetireOOO, WriteProcResIdx,#, WriteLatencyIdx,#, ReadAdvanceIdx,#}
>   static const llvm::MCSchedClassDesc SchedModel_ASchedClasses[] = {
>     {DBGFIELD("InvalidSchedClass")  8191, false, false, false, 0, 0,  0, 0,  0, 0},
>     {DBGFIELD("Inst_A")             1, false, false, false,  0, 0,  1, 1,  0, 0}, // #1
>     {DBGFIELD("Inst_B")             1, false, false, false,  0, 0,  1, 1,  0, 0}, // #2
>     {DBGFIELD("Inst_C")             1, false, false, false,  0, 0,  1, 1,  1, 1}, // #3
>   }; // SchedModel_ASchedClasses
>
> Based on that last descriptor, Inst_C declares a single read-advance entry (i.e. entry #1 in `MyTargetReadAdvanceTable`).
> Entry #1 in `MyTargetReadAdvanceTable` declares a 1cy of read advance when associated with Write_B.
>
> In conclusion, for completeness, you need to check also tables `MyTargetReadAdvanceTable` and `SchedModel_ASchedClasses`.
>
> I hope it helps.

Thank you a lot for so detail comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132205/new/

https://reviews.llvm.org/D132205