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.

Sign up for GitHub

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

Jump to bottom

[AIEX] Fix ExpandPostRAPseudos and PostSelectOptimize passes to satisfy MachineVerifier #349

Merged
merged 3 commits into from
Feb 13, 2025
Merged

[AIEX] Fix ExpandPostRAPseudos and PostSelectOptimize passes to satisfy MachineVerifier #349

merged 3 commits into from
Feb 13, 2025

Conversation

khallouh
Copy link
Collaborator

No description provided.

@khallouh khallouh force-pushed the hamza.satisfy.machine.verifier branch from e685ee2 to 33118de Compare February 12, 2025 17:49
andcarminati
andcarminati reviewed Feb 12, 2025
View reviewed changes
llvm/lib/Target/AIE/AIEPostSelectOptimize.cpp Outdated
// AIE's PseudoMove instruction takes compound register classes which
// contains registers of different sizes. We need to use the right classes
// to avoid the MachineVerifier complaining about mismatching sizes.
auto ConstrainAddrRegClass = [&](Register Reg) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just:

return (MRI.constrainRegClass(Reg, TRI->getAddrAs32BitDCRegClass()) ||
          MRI.constrainRegClass(Reg, TRI->getAddrAs32BitDNRegClass()) ||
          MRI.constrainRegClass(Reg, TRI->getAddrAs32BitDJRegClass()) ||
          MRI.constrainRegClass(Reg, TRI->getAddrAs32BitMRegClass()) ||
          MRI.constrainRegClass(Reg, TRI->getAddrAs32BitPRegClass()));

andcarminati
andcarminati reviewed Feb 12, 2025
View reviewed changes
llvm/lib/Target/AIE/aie2p/AIE2PInstrInfo.cpp Outdated
@@ -1162,8 +1162,7 @@ bool AIE2PInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
case AIE2P::PseudoMove: {
Register Dst = MI.getOperand(0).getReg();
Register Src = MI.getOperand(1).getReg();
BuildMI(MBB, MI, DL, get(AIE2P::MOV_alu_mv_mv_mv_scl), Dst)
.addReg(Src, getKillRegState(true));
BuildMI(MBB, MI, DL, get(AIE2P::MOV_alu_mv_mv_mv_scl), Dst).addReg(Src);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about keeping the existing state, like getKillRegState(MI.getOperand(1).isKill()) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can't steal/copy the entire Src operand, even if we throw it away in the next line?

andcarminati
andcarminati reviewed Feb 12, 2025
View reviewed changes
llvm/lib/Target/AIE/AIEPostSelectOptimize.cpp Outdated
MRI.constrainRegClass(Reg, TRI->getAddrAs32BitDNRegClass()) ||
MRI.constrainRegClass(Reg, TRI->getAddrAs32BitDJRegClass()) ||
MRI.constrainRegClass(Reg, TRI->getAddrAs32BitMRegClass()) ||
MRI.constrainRegClass(Reg, TRI->getAddrAs32BitPRegClass()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think M, N and J are enough here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed P but kept C

@andcarminati
Copy link
Collaborator

Nice investigation! I would like recommend two things:

  1. Update the dates in the test headers.
  2. Enable machine verifier on the tests.

martien-de-jong
martien-de-jong reviewed Feb 13, 2025
View reviewed changes
llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vconv-bfp16.mir
@@ -28,7 +28,7 @@ body: |
%1:accregbank(<64 x s32>) = COPY $dm0
%2:vregbank(<64 x s8>) = COPY $x1
%3:gprregbank(<8 x s8>) = COPY $e1
%0:vregbank(<64 x s8>), %4:gprregbank(<8 x s8>) = G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.aie2p.v64accfloat.to.v64bfp16ebs16), %2(<64 x s8>), %3(<8 x s8>), %1(<64 x s32>)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Random copy paste junk? Do we still need %3 and %1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this test was wrong. I fixed it. The input should have been a copy of $dm0 not $x1. the verify doesn't detect such problems :/

martien-de-jong
martien-de-jong reviewed Feb 13, 2025
View reviewed changes
llvm/lib/Target/AIE/AIEBaseRegisterInfo.h Outdated
}
virtual const TargetRegisterClass *getAddrAs32BitDCRegClass() const {
llvm_unreachable("Target didn't implement getAddrAs32BitDCRegClass!");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It looks as if these may be AIE2XX specific. Perhaps put the entire constrain sequence in a virtual method, like constrainSubwordRegister?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this idea. +1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@khallouh khallouh force-pushed the hamza.satisfy.machine.verifier branch from 33118de to c9293ea Compare February 13, 2025 10:30
gbossu
gbossu reviewed Feb 13, 2025
View reviewed changes
llvm/lib/Target/AIE/AIEPostSelectOptimize.cpp Outdated
// contains registers of different sizes. We need to use the right classes
// to avoid the MachineVerifier complaining about mismatching sizes.
assert(TRI->constrainAddrRegClass(MRI, Reg) &&
"Expected an addressing register class");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the need for a new hook. Why can't we do the same as in ISel and constrain the operands to the largest superclass of the existing class, and the one in the instruction descriptor? We would then get a 32-bit class, because MvScl class is a compound 32-bit class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. I updated to use constrainSelectedInstRegOperands

@khallouh khallouh force-pushed the hamza.satisfy.machine.verifier branch 2 times, most recently from 73599c8 to 7e17de8 Compare February 13, 2025 11:38
@khallouh khallouh force-pushed the hamza.satisfy.machine.verifier branch from 7e17de8 to ed08029 Compare February 13, 2025 12:34
andcarminati
andcarminati approved these changes Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@andcarminati andcarminati left a comment

Choose a reason for hiding this comment

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

LGTM.

@khallouh khallouh merged commit 5a06035 into aie-public Feb 13, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbossu gbossu gbossu left review comments

@martien-de-jong martien-de-jong martien-de-jong left review comments

@andcarminati andcarminati andcarminati approved these changes

@abhinay-anubola abhinay-anubola Awaiting requested review from abhinay-anubola abhinay-anubola is a code owner

@abnikant abnikant Awaiting requested review from abnikant abnikant is a code owner

@F-Stuckmann F-Stuckmann Awaiting requested review from F-Stuckmann F-Stuckmann is a code owner

@katerynamuts katerynamuts Awaiting requested review from katerynamuts katerynamuts is a code owner

@konstantinschwarz konstantinschwarz Awaiting requested review from konstantinschwarz konstantinschwarz is a code owner

@niwinanto niwinanto Awaiting requested review from niwinanto niwinanto is a code owner

@SagarMaheshwari99 SagarMaheshwari99 Awaiting requested review from SagarMaheshwari99 SagarMaheshwari99 is a code owner

@stephenneuendorffer stephenneuendorffer Awaiting requested review from stephenneuendorffer stephenneuendorffer is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@khallouh @andcarminati @gbossu @martien-de-jong