Skip to content
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

[AIEX] NFC: Refactor Alternate Descriptor codebase #193

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

krishnamtibrewala
Copy link
Collaborator

No description provided.

@krishnamtibrewala krishnamtibrewala force-pushed the aie-altdescriptor branch 3 times, most recently from 51fe0dd to 9f8a55f Compare September 23, 2024 17:18
@gbossu
Copy link
Collaborator

gbossu commented Oct 1, 2024

I'm starting to wonder how far we should go with adding more layers in the schedule. I'd say we need to go even further. Our interfaces to the HazardRecognizer are becoming painful to work with. I'd like group together what is really needed to make a correct scheduling decision. I'd say at this point we need something like:

struct DynamicMIDescriptor {
  ArrayRef<InstrStage> ItinStages;
  SlotBits SlotSet,
  MemoryBankBits MemoryBanks,
  SmallVector<int, 2> MemoryAccessCycles
};

Then we could just pass that dynamic descriptor to the checkHazard/enterResources methods of the HazardRecognizer. Within the scheduler, we could easily retrieve that dynamic descriptor by doing something like AlternateDescrMap->getDynamicDescr(MI), and that would find the right itinerary, slotset, etc. all based on the selected opcode (or the standard one).

I don't think we should do that change just now, and I can also work on this, but I'd like to start the discussion. What do you think @martien-de-jong @krishnamtibrewala @andcarminati ?

@martien-de-jong
Copy link
Collaborator

martien-de-jong commented Oct 1, 2024

I'm starting to wonder how far we should go with adding more layers in the schedule. I'd say we need to go even further. Our interfaces to the HazardRecognizer are becoming painful to work with. I'd like group together what is really needed to make a correct scheduling decision. I'd say at this point we need something like:

struct DynamicMIDescriptor {
  ArrayRef<InstrStage> ItinStages;
  SlotBits SlotSet,
  MemoryBankBits MemoryBanks,
  SmallVector<int, 2> MemoryAccessCycles
};

Then we could just pass that dynamic descriptor to the checkHazard/enterResources methods of the HazardRecognizer. Within the scheduler, we could easily retrieve that dynamic descriptor by doing something like AlternateDescrMap->getDynamicDescr(MI), and that would find the right itinerary, slotset, etc. all based on the selected opcode (or the standard one).

I don't think we should do that change just now, and I can also work on this, but I'd like to start the discussion. What do you think @martien-de-jong @krishnamtibrewala @andcarminati ?

That looks good. I would then also like a constructor for this that takes a MachineInstr. I can also imagine that we grab the static part of this and let tablegen generate it.

@gbossu
Copy link
Collaborator

gbossu commented Oct 1, 2024

That looks good, except that all of this is static, not dynamic ;-)

Fair enough 😆 I thought of dynamic as in "the scheduling descriptor is determined based on the selected opcode, and operands". Those things can change throughout the compilation process. Maybe you can think of a better qualifier than "dynamic"? Maybe "FullyQualifiedDescr"?

@martien-de-jong
Copy link
Collaborator

That looks good, except that all of this is static, not dynamic ;-)

Fair enough 😆 I thought of dynamic as in "the scheduling descriptor is determined based on the selected opcode, and operands". Those things can change throughout the compilation process. Maybe you can think of a better qualifier than "dynamic"? Maybe "FullyQualifiedDescr"?

I guess that MemoryBanks is dynamic

@krishnamtibrewala krishnamtibrewala force-pushed the aie-altdescriptor branch 3 times, most recently from 1eddb47 to 44e273f Compare October 2, 2024 11:20
@krishnamtibrewala krishnamtibrewala force-pushed the aie-altdescriptor branch 3 times, most recently from efffa0b to 0b41f3d Compare October 2, 2024 13:40
@andcarminati
Copy link
Collaborator

I'm starting to wonder how far we should go with adding more layers in the schedule. I'd say we need to go even further. Our interfaces to the HazardRecognizer are becoming painful to work with. I'd like group together what is really needed to make a correct scheduling decision. I'd say at this point we need something like:

struct DynamicMIDescriptor {
  ArrayRef<InstrStage> ItinStages;
  SlotBits SlotSet,
  MemoryBankBits MemoryBanks,
  SmallVector<int, 2> MemoryAccessCycles
};

Then we could just pass that dynamic descriptor to the checkHazard/enterResources methods of the HazardRecognizer. Within the scheduler, we could easily retrieve that dynamic descriptor by doing something like AlternateDescrMap->getDynamicDescr(MI), and that would find the right itinerary, slotset, etc. all based on the selected opcode (or the standard one).

I don't think we should do that change just now, and I can also work on this, but I'd like to start the discussion. What do you think @martien-de-jong @krishnamtibrewala @andcarminati ?

I agree that HR is handling several different interfaces now and this redesign will help to concentrate all the information that is related to one specific instruction. We can put this on our backlog for sure.

Copy link
Collaborator

@gbossu gbossu left a comment

Choose a reason for hiding this comment

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

LGTM

@krishnamtibrewala krishnamtibrewala enabled auto-merge (rebase) October 4, 2024 14:13
@krishnamtibrewala krishnamtibrewala merged commit 0e2d85b into aie-public Oct 4, 2024
8 checks passed
@krishnamtibrewala krishnamtibrewala deleted the aie-altdescriptor branch October 5, 2024 03:28
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