[NFC][AIE] Streamline the format checks using Conflict bits. #704
Conversation
martien-de-jong
requested review from
F-Stuckmann,
SagarMaheshwari99,
abhinay-anubola,
abnikant,
andcarminati,
katerynamuts,
khallouh,
konstantinschwarz,
mludevid,
niwinanto and
stephenneuendorffer
as code owners
|
|
||
| // Mark a slot as artificial. This will exclude it from any resource | ||
| // estimations. | ||
| bit Artificial = false; |
There was a problem hiding this comment.
This is used for e.g. nop slots that are just added to drive format selection, but don't occupy existing slots.
| // X -> {XM}, | ||
| // M -> {XM}, | ||
| // XM -> {X, M, XM} | ||
| // and nothing more. In particular, ConflictBits of A should be equal to its SlotBits |
There was a problem hiding this comment.
I'll extend this test a bit.
martien-de-jong
force-pushed
the
martien.slotbits
branch
2 times, most recently
from
cb5c72a to
c55211a
Compare
| // CHECK-LABEL: "X", | ||
| // CHECK-NEXT: 8, | ||
| // CHECK-NEXT: 1, | ||
| // CHECK-NEXT: 5, |
There was a problem hiding this comment.
size=8, slot=0001, conflicts=0101
martien-de-jong
force-pushed
the
martien.slotbits
branch
from
c55211a to
80982bc
Compare
llvm/lib/Target/AIE/AIEBundle.h
Outdated
| if (OccupiedSlots & ConflictBits) { | ||
| return false; | ||
| } | ||
| SlotBits NewSlots = OccupiedSlots | SlotInfo->getSlotSet(); |
There was a problem hiding this comment.
If we are using ConflictBits, why do we still need to check the format (getFormat)?
There was a problem hiding this comment.
Assert? and return true?
There was a problem hiding this comment.
Or even a comment to clarify.
martien-de-jong
force-pushed
the
martien.slotbits
branch
4 times, most recently
from
858edf4 to
89e24e9
Compare
| auto *SlotInfo = FormatInterface->getSlotInfo(Slot); | ||
| // ConflictBits is a fast predictor of missing formats | ||
| SlotBits ConflictBits = SlotInfo->getConflictSet(); | ||
| if (OccupiedSlots & ConflictBits) { |
There was a problem hiding this comment.
wouldn't the OccupiedSlots be the ConflictBits of a Bundle?
There was a problem hiding this comment.
I think all your comments are covered by this one answer. We keep Slots since they represent the true base class from which the instructions derive. Each instruction has exactly one slot. These are also the slots that are listed in the ISA, in the formats, they define the operands of the format instructions, etc.
The conflicts have no such direct interpretation. I use them for which they were designed, which is as an easy way to detect conflicts given the regular slot occupation bits without scanning the format table.
They end up in FuncUnitWrapper since that is at the heart of the conflict query interface. I cannot enter them in the ScoreBoard, since both X and M would set the XM bit, which would not allow X to be in the same bundle as M.
| /// Opcode of the NOP inst. attached to the slot | ||
| /// The closure of SlotOccupancy with the computed exclusions, | ||
| /// e.g. XM implies X and M | ||
| const SlotBits ConflictBits; |
There was a problem hiding this comment.
why do we need a new SlotBits?
Could we not have a unified SlotOccupancy where XM is properly mapped to X and M occupation?
| bool FuncUnitWrapper::operator==(const FuncUnitWrapper &Other) const { | ||
| return Required == Other.Required && Reserved == Other.Reserved && | ||
| Slots == Other.Slots && MemoryBanks == Other.MemoryBanks && | ||
| Slots == Other.Slots && Conflicts == Other.Conflicts && |
There was a problem hiding this comment.
why do we have to check slots and conflicts separately?
|
Overall I do not understand why we still carry around the regular |
| MemoryBankBits MemoryBanks = 0, MemoryObjectsBits ObjectsBits = 0, | ||
| SmallVector<int, 2> MemoryAccessCycles = {}) { | ||
| return checkConflict(MockScoreboard, &Itins, SchedClass, SlotSet, | ||
| return checkConflict(MockScoreboard, &Itins, SchedClass, SlotSet, SlotSet, |
There was a problem hiding this comment.
shouldn't we be passing conflictbits here?
There was a problem hiding this comment.
This is a small existing mockup where they coincide.
| // the slot and format details. | ||
| return Slots && Other.Slots && | ||
| !FormatInterface->getPacketFormats().getFormat(Slots | Other.Slots); | ||
| !FormatInterface->isFormatAvailable(Slots | Other.Slots); |
There was a problem hiding this comment.
why aren't we checking conflict bits here?
There was a problem hiding this comment.
we are, at line 131, returning early if we find a conflict.
martien-de-jong
force-pushed
the
martien.slotbits
branch
from
89e24e9 to
b5a52e5
Compare
ConflictBits summarize conflicts between one slot and another, in particular between e.g. XM and X and M
Availability of a format with a particular slot occupation is summarized in a lookup table with the slot set as index.
I have a hunch that the format check is actually not necessary anymore, but now that it is O(1), it doesn't matter much. The reasoning is that the 128 bit format can accommodate all combinations of real slots, and the conflict bits now catch the artificial composite slot constraints.