Skip to content

Stuckmann.multi.slot.pseudos #304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

F-Stuckmann
Copy link
Collaborator

@F-Stuckmann F-Stuckmann commented Jan 23, 2025

Statically assign multi-slot pseudo instructions.

todos:

  • do not refactor SlotCounts into an own utilitiy function

@F-Stuckmann F-Stuckmann force-pushed the stuckmann.multi.slot.pseudos branch 2 times, most recently from 1b09f1e to 6f5a268 Compare February 5, 2025 10:15
@F-Stuckmann F-Stuckmann force-pushed the stuckmann.multi.slot.pseudos branch 2 times, most recently from 7dc8b43 to 2537cb3 Compare February 10, 2025 08:54
@F-Stuckmann
Copy link
Collaborator Author

F-Stuckmann commented Feb 10, 2025

  • I moved the multi slot Assignment into the leaveBlock stage of leaveMBB, so Machine Scheduling happens on the materialized VLD_pseudo Instructions (i.e. VLDA/VLDB)
  • I left the getMemoryBanks in the AIEHazardRecognizer and passed a Reference of AIEHazardRecognizer into my function call.
  • I now collect all the addrspace and Slots, decide an assign, and then set the Descriptor of the MachineInstr

@F-Stuckmann F-Stuckmann marked this pull request as ready for review February 10, 2025 13:50
@@ -60,4 +60,8 @@ const MCFormatDesc *AIEMCFormats::getMCFormats() const { return AIE::Formats; }

const PacketFormats &AIEMCFormats::getPacketFormats() const { return Formats; }

SmallVector<MCSlotKind, 2> AIEMCFormats::getLoadSlotKinds() const {
return {AIESlotKind::AIE_SLOT_LDB, AIESlotKind::AIE_SLOT_LDA};
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to forget AIE1. We won't hit the unreachable, will we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would hit unreachable, if i don't implement it for AIE1 and if the option is turned on by default, we would break stuff

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we? I don't think AIE1 uses the MachineScheduler

@F-Stuckmann F-Stuckmann force-pushed the stuckmann.multi.slot.pseudos branch from 27a6f34 to c2bc7d9 Compare February 18, 2025 17:54
Copy link
Collaborator

@martien-de-jong martien-de-jong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Apply nits to taste

/// \return an unused Slot from the mapping.
std::optional<MCSlotKind> getUnusedSlot(const AIEBaseInstrInfo *TII) const {
const SmallVector<MCSlotKind, 2> UnusedLoadSlots =
TII->getFormatInterface()->getLoadSlotKinds();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: just LoadSlots?

Nit2: Maybe also rename the function to getUnusedLoadSlot(), because this is only to be used for multi-slot instructions that load.

It might actually just be better to iterate over the slots of the multi-slot instruction directly? I'm not sure why we need getLoadSlotKinds()

std::optional<MCSlotKind>
getLeastRecentlyUsedSlot(const AIEBaseInstrInfo *TII) {
SmallVector<MCSlotKind, 2> AvailableSlots =
TII->getFormatInterface()->getLoadSlotKinds();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: I guess this is fine for now, but I believe we'd rather just collect all the "feasible slots" by iterating over the multi-slot instructions, and create a least-recently-used list out of them, like we do in register-reallocation. This would avoid the need for a new getLoadSlotKinds() hook.

@gbossu gbossu force-pushed the gaetan.postpipeliner.iterative branch from 6af25cb to 584bec1 Compare February 19, 2025 11:01
@gbossu
Copy link
Collaborator

gbossu commented Feb 19, 2025

Heads-up: I have now rebased #359 on top of aie-public. I think you can rebase as well.

Also, it would be nice to update the tests in llvm/test/CodeGen/AIE/aie2p/end-to-end/ so that they use --aie-multi-slot-pseudo-instr

@F-Stuckmann F-Stuckmann force-pushed the stuckmann.multi.slot.pseudos branch 2 times, most recently from 89f3c1d to e76ea94 Compare February 19, 2025 13:45
@gbossu gbossu force-pushed the gaetan.postpipeliner.iterative branch from 584bec1 to d10386e Compare February 19, 2025 16:22
@F-Stuckmann F-Stuckmann force-pushed the stuckmann.multi.slot.pseudos branch from e9d83bf to cba1294 Compare February 20, 2025 09:47
@F-Stuckmann
Copy link
Collaborator Author

fixed githubs rebase issues

@gbossu gbossu deleted the branch Xilinx:gaetan.postpipeliner.iterative February 20, 2025 10:24
@gbossu gbossu closed this Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants