NFC changes to make TableGen files a bit more readable, by using loops instead of a lot of similar defs with just e.g. a latency value that changes.
Details
Diff Detail
Event Timeline
Thanks for helping me out with TableGen coding, Javed!
But you have LSU, LSU2, LSU3, ... . The first one would have to change to LSU1
I have sofar tried to avoid LSU1, FXa1, so I did
foreach Num = ["", "2", "3", "4", "5"]
, which seems to work. I would think this is ok, or? Only drawback here is that now there are in the subtarget files a separate definition for the "implicit 1\
" in some places. Not sure which is better...
Compared to trunk, there are in the tablegen file some extra variants, like FXa5 in z14 file, but that did not seem to matter - it seems that the produced cla\
ng binary is identical to that of trunk.
Any suggestions most welcome!
Hi Jonas:
To avoid unnecessary defs (for dis-continous ranges), you can use range list like this:
foreach L = {1-3,6-8} in {
def "WLat"#L : SchedWrite;
}
I tried it out and it works.
Thanks, Javed. Still, it seemed that the clang binary was identically which should mean that any unused entries are simply ignored and left out from the generated tables. So perhaps using combined ranges per above is mostly cosmetical?
Yes Jonas it is purely cosmetic. Sometimes maybe be useful for code maintenance to show that certain records/values are not-possible
I believe in this case it would be preferable to include the full range of latency values. We should try to keep the source as concise and straightforward as possible; if this results in some units being defined that aren't used, that doesn't seem to be a drawback to me. Also, for the common code parts, even if no current instruction uses any particular value, there may always be additional instructions in future processors.
For some reason, the built clang binary (with the patch of a few days ago) makes some noise with diff now , but the binaries are of the exact same size, and SPEC output is identical...
Per off-line discussions I have now tried to simplify further the tablegen loops by including in some cases extra unused SchedWrites, e.g. VecBF3, etc. The binary size and SPEC output are still identical, so this looks fine.
We could make the loops even neater by using FXAa1, FXb1, etc... but then we also get a lot of FXa1 with the instructions, which I think is not quite as neat. Personally, I think it looks good as it is now without the FXa1...
See inline comments. In general I agree with this approach.
lib/Target/SystemZ/SystemZSchedule.td | ||
---|---|---|
30 | I believe you can omit the braces if there's just a single statement inside the foreach. | |
55 | Why isn't this in the loop above as well? | |
58–59 | I'd probably just but all of them in the loop too. Except the VecFPd, which is special. |
Tried to use clang-format a bit on this file.. Does it look ok, or should I change back? Thinking about the WLatLSU loop and the first/last spaces in the Num = [ "", .. "" ].
A few more comments, LGTM with those changes.
lib/Target/SystemZ/SystemZScheduleZ13.td | ||
---|---|---|
64 | Braces here as well. | |
85–86 | For consistency with the general definition, you should move those (except VecFDd) up in the loop as well. | |
lib/Target/SystemZ/SystemZScheduleZ14.td | ||
64 | Same comment as for z13. | |
85–86 | Same comment as for z13. |
Sorry, missed your last update.
I'm not sure clang-format understands .td files well. This case:
foreach L = 1 - 16 in def "WLat"#L#"LSU" : WriteSequence<[ !cast<SchedWrite>("WLat"#L), LSULatency ]>;
does indeed look a bit weird, I'd probably write something like:
foreach L = 1 - 16 in def "WLat"#L#"LSU" : WriteSequence<[ !cast<SchedWrite>("WLat"#L), LSULatency ]>;
I believe you can omit the braces if there's just a single statement inside the foreach.