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

[AIE2P] Refactor register bank selection #325

Open
wants to merge 1 commit into
base: aie-public
Choose a base branch
from

Conversation

niwinanto
Copy link
Collaborator

No description provided.

@niwinanto niwinanto force-pushed the niwin.regbank.select branch from e68a4c6 to 3f14d94 Compare January 31, 2025 11:10
});
}

const RegisterBank *AIE2PRegisterBankInfo::getPreferredRegBankForVectorTy(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we have an optional getPreferredPartialMappingIdx(Type)? In this way we can merge a bunch of fifo acc code.

isAccRegMapping = true;
if (isUseFifoInsn(MRI, TRI, UseCandidate))
if (&AIE2P::FifoRegBank == PreferredRB)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We the optional approach, we can simplify a lot this case.

@niwinanto niwinanto force-pushed the niwin.regbank.select branch from 3f14d94 to 9ac5133 Compare February 5, 2025 11:16
@niwinanto niwinanto marked this pull request as ready for review February 5, 2025 11:22
@niwinanto niwinanto changed the title [WIP][AIE2P] Refactor register bank selection [AIE2P] Refactor register bank selection Feb 5, 2025
return hasFifoInput(UseMI, MRI, TRI, RegOp);
});
// Trace Reg to its actual register, updating it if necessary
TraceToActualReg(Reg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: we currently don't have a test for this for the fifo bank because we weren't tracing the register. @andcarminati also implements it in #314 plus ignoring PHI nodes as well, so whichever PR go in first has to add the tests.

});
}

bool AIE2PRegisterBankInfo::isUsedAsFifoReg(const MachineRegisterInfo &MRI,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I got some suggestions regarding naming these functions after I had merged this #288 Can you check them and take them into account in this PR?

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.

3 participants