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

[GlobalISel][AArch64] Combine unmerge(G_EXT v, undef) to unmerge(v). #65263

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

dzhidzhoev
Copy link
Member

When having <N x t> d1, unused = unmerge(G_EXT <2*N x t> v1, undef, N), it is possible to express it just as unused, d1 = unmerge v1.

It is useful for tackling regressions in arm64-vcvt_f.ll, introduced in https://reviews.llvm.org/D144670.

@arsenm
Copy link
Contributor

arsenm commented Sep 4, 2023

Should be a generic combine. Can you generalize to N unused defs?

@dzhidzhoev
Copy link
Member Author

Should be a generic combine. Can you generalize to N unused defs?

G_EXT is an opcode for AArch64's ext instruction. Could you please suggest how to generalize it for other targets?

Copy link
Contributor

@aemerson aemerson left a comment

Choose a reason for hiding this comment

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

I think this needs some MIR tests.

if (!DstTy.isVector())
return false;

MachineInstr *Ext = getDefIgnoringCopies(
Copy link
Contributor

Choose a reason for hiding this comment

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

getOpcodeDef() with the GUnmerge source reg helper here?

if (!LowestVal || LowestVal->Value.getZExtValue() != DstTy.getSizeInBytes())
return false;

MachineInstr *Undef = getDefIgnoringCopies(ExtSrc2, MRI);
Copy link
Contributor

Choose a reason for hiding this comment

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

getOpcodeDef()

@arsenm
Copy link
Contributor

arsenm commented Sep 5, 2023

Should be a generic combine. Can you generalize to N unused defs?

G_EXT is an opcode for AArch64's ext instruction. Could you please suggest how to generalize it for other targets?

G_EXT should be G_AARCH64_EXT then. I assumed this was shorthand for all 3 extends

Copy link
Contributor

@aemerson aemerson left a comment

Choose a reason for hiding this comment

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

LGTM with minor changes. We can actually rename the G_EXT and friends to specify AArch64 as well in another change.

return false;
if (!MRI.use_nodbg_empty(MI.getOperand(1).getReg()))
if (!MRI.use_nodbg_empty(Unmerge.getOperand(1).getReg()))
Copy link
Contributor

Choose a reason for hiding this comment

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

GenericMachineInstr also gives you getReg(N)

@@ -1071,18 +1071,18 @@ void applyVectorSextInReg(MachineInstr &MI, MachineRegisterInfo &MRI,
bool matchUnmergeExtToUnmerge(MachineInstr &MI, MachineRegisterInfo &MRI,
Register &MatchInfo) {
assert(MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES);
Copy link
Contributor

Choose a reason for hiding this comment

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

This assert becomes unnecessary once you have the cast<GUnmerge> below.

When having <N x t> d1, unused = unmerge(G_EXT <2*N x t> v1, undef, N),
it is possible to express it just as unused, d1 = unmerge v1.

It is useful for tackling regressions in arm64-vcvt_f.ll, introduced in
https://reviews.llvm.org/D144670.
@dzhidzhoev dzhidzhoev merged commit bb1a03d into llvm:main Sep 5, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants