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

Support for allowing direct VEXTRACT to 20-bit registers #233

Open
wants to merge 2 commits into
base: aie-public
Choose a base branch
from
Open

Support for allowing direct VEXTRACT to 20-bit registers #233

wants to merge 2 commits into from

Conversation

abhinay-anubola
Copy link
Collaborator

@abhinay-anubola abhinay-anubola commented Nov 8, 2024 •
edited
Loading

  • This update introduces a new generic combiner that simplifies the sequence sext(trunc x) directly to x when applicable.
  • Added VExtract combiner that enables above generic combiner, thus we have 20-bit vextract.
  • The MachineVerifier has been updated to allow G_AIE_SEXT_EXTRACT_VECTOR_ELT and G_AIE_ZEXT_EXTRACT_VECTOR_ELT to accept 20-bit outputs.
  • Additionally, tests have been added and updated to reflect these functional changes.

@krishnamtibrewala
Copy link
Collaborator

Given that you mentioned there are no QoR gain, I would recommend you to re look at the instruction that consume S20 type reg.
Because for the optimization starts to trace back from an instruction that consumes S20 type which might not be captured in isNativeS20Consumer function.

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 650d8a9 to d5d7cf0 Compare November 12, 2024 11:21
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from d5d7cf0 to f12f1a4 Compare November 14, 2024 09:13
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch 2 times, most recently from 11cd993 to 35027af Compare December 13, 2024 06:32
@abhinay-anubola abhinay-anubola mentioned this pull request Dec 13, 2024
Merged
andcarminati
andcarminati reviewed Dec 16, 2024
View reviewed changes
llvm/lib/Target/AIE/AIE2PreLegalizerCombiner.cpp
MIRBuilder.buildAssertInstr(AssertExtOpcode, ExtReg20Bit, DstReg20Bit,
SrcEltSize);
MIRBuilder.buildInstr(ExtOpcode, {DstReg}, {ExtReg20Bit});
MI.eraseFromParent();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now we are safe ;-)

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 35027af to 000b09e Compare December 17, 2024 05:58
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 000b09e to e7731e6 Compare December 17, 2024 09:08
gbossu
gbossu reviewed Dec 30, 2024
View reviewed changes
llvm/test/CodeGen/RISCV/GlobalISel/jumptable.ll
@@ -192,7 +190,6 @@ define void @above_threshold(i32 signext %in, ptr %out) nounwind {
; RV64I-PIC-LABEL: above_threshold:
; RV64I-PIC: # %bb.0: # %entry
; RV64I-PIC-NEXT: li a2, 5
; RV64I-PIC-NEXT: sext.w a0, a0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Did you check all targets?

Copy link
Collaborator Author

@abhinay-anubola abhinay-anubola Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I have checked all targets.

gbossu
gbossu reviewed Dec 30, 2024
View reviewed changes
llvm/lib/Target/AIE/AIE2PreLegalizerCombiner.cpp
/// To : %9:_(s20) = G_AIE_SEXT_EXTRACT_VECTOR_ELT %2(<32 x s16>), %0(s32)
/// %10:_(s20) = G_ASSERT_[S/Z]EXT %9, 16
/// %4:_(s16) = G_TRUNC %10(s20)
/// %5:_(s20) = G_[S/Z]EXT %4(s16)
Copy link
Collaborator

@gbossu gbossu Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to change the return types? I would expect that we only need to add a %10:_(s32) = G_ASSERT_[S/Z]EXT %9, 16 and keep the rest intact thanks to the new sext(trunc x) combiner you added previously.

Copy link
Collaborator Author

@abhinay-anubola abhinay-anubola Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we need to change the return types, because the pattern that is written in new sext(trunc x) combiner will not match in this case as m_SpecificType is trying to match s20 but return type here is s32.
mi_match(SrcReg, MRI, m_GTrunc(m_all_of(m_Reg(Reg), m_SpecificType(DstTy))))

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from e7731e6 to 467672d Compare January 2, 2025 10:54
andcarminati
andcarminati previously approved these changes Jan 2, 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.

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 2d5aaad to de8468e Compare January 8, 2025 10:42
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from de8468e to c3cba3e Compare February 20, 2025 09:52
konstantinschwarz
konstantinschwarz reviewed Feb 20, 2025
View reviewed changes
llvm/test/CodeGen/AMDGPU/GlobalISel/combine-sext-trunc.mir
%var:_(s32) = COPY $vgpr0
%assert:_(s32) = G_ASSERT_SEXT %var, 16
%trunc:_(s16) = G_TRUNC %assert(s32)
%sext:_(s64) = G_SEXT %trunc(s16)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I guess we could still eliminate the G_TRUNC, and change G_SEXT to extend from 32 -> 64?

llvm/lib/Target/AIE/aie2p/AIE2PInstrInfo.cpp
ErrInfo = "Expected 32/64bit scalar destination";
return MRI.getType(MI.getOperand(0).getReg()) == LLT::scalar(32) ||
MRI.getType(MI.getOperand(0).getReg()) == LLT::scalar(64);
ErrInfo = "Expected 20/32/64bit scalar destination";
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we haven't changed anything for AIE2p, we still expect only 32-/64-bit destination types

llvm/lib/Target/AIE/AIE2PreLegalizerCombiner.cpp
const LLT S20 = LLT::scalar(20);
Register DstReg20Bit = MRI.createGenericVirtualRegister(S20);
Register ExtReg20Bit = MRI.createGenericVirtualRegister(S20);
MachineIRBuilder MIRBuilder(MI);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Combiner class already has a MachineIRBuilder member B, we should use it here to make us of potential CSE opportunities: initialize as B.setInstrAndDebugLoc(MI); and then use below

llvm/lib/Target/AIE/AIE2InstrInfo.cpp
@@ -159,8 +159,9 @@ bool AIE2InstrInfo::verifyGenericInstruction(const MachineInstr &MI,
return false;
}
}
ErrInfo = "Expected 32bit scalar destination";
return MRI.getType(MI.getOperand(0).getReg()) == LLT::scalar(32);
ErrInfo = "Expected 20/32bit scalar destination";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add instruction selection tests for s20 = G_AIE_[SZ]EXT_EXTRACT_VECTOR_ELT now that we made these new types legal?

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

@konstantinschwarz konstantinschwarz konstantinschwarz requested changes

@krishnamtibrewala krishnamtibrewala krishnamtibrewala left review comments

@andcarminati andcarminati andcarminati left review comments

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

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

@martien-de-jong martien-de-jong Awaiting requested review from martien-de-jong martien-de-jong 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

@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

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

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
5 participants
@abhinay-anubola @krishnamtibrewala @andcarminati @gbossu @konstantinschwarz