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

[AArch64][GlobalISel] Look through COPY and G_BITCAST while selecting fcvtl2 (fpext) #65463

Closed
wants to merge 0 commits into from

Conversation

dzhidzhoev
Copy link
Member

It tackles some regressions introduced in
https://reviews.llvm.org/D144670.

@dzhidzhoev dzhidzhoev requested a review from a team as a code owner September 6, 2023 11:03
@dzhidzhoev dzhidzhoev requested a review from a team September 6, 2023 11:20
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.

def : Pat<(v4f32 (int_aarch64_neon_vcvthf2fp (v4i16 V64:$Rn))),
(FCVTLv4i16 V64:$Rn)>;
def : Pat<(v4f32 (int_aarch64_neon_vcvthf2fp (extract_subvector (v8i16 V128:$Rn),
(i64 4)))),
(FCVTLv8i16 V128:$Rn)>;
def : Pat<(v2f64 (any_fpextend (v2f32 V64:$Rn))),
(FCVTLv2i32 V64:$Rn)>;
def : Pat<(v2f64 (any_fpextend (v2f32 (extract_high_v4f32 (v4f32 V128:$Rn))))),
def : PatIgnoreCopies<(v2f64 (any_fpextend (v2f32 (extract_high_v4f32 (v4f32 V128:$Rn))))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed? Do you have a test case where it makes a difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I've mixed it up with another change.

Root.getReg() == Extract->getOperand(1).getReg()) {
Register ExtReg = Extract->getOperand(2).getReg();
auto Extract = getDefSrcRegIgnoringCopies(Root.getReg(), MRI);
while (Extract && Extract->MI->getOpcode() == TargetOpcode::G_BITCAST)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work for big endian? I'm not sure if big endian works for global isel at the moment, but I would prefer not to introduce a lot of subtle cases that assume bitcast does not alter lane layout.

Copy link
Contributor

Choose a reason for hiding this comment

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

Big endian currently doesn't work, we have known issues but you're right we should do a LE check here.

return {{[=](MachineInstrBuilder &MIB) { MIB.addUse(ExtReg); }}};
}
}
if (Extract->MI->getOpcode() == TargetOpcode::G_EXTRACT_VECTOR_ELT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is every G_EXTRACT_VECTOR_ELT considered a high extract? Should it be checking that the Extract is a v2s64 and is extracting from index is 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the catch!

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

auto LaneIdx = getIConstantVRegValWithLookThrough(
Extract->MI->getOperand(2).getReg(), MRI);
if (LaneIdx &&
SrcTy == LLT::vector(ElementCount::getFixed(2), LLT::scalar(64)) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

SrcTy == LLT::fixed_vector(2, 64) I think would work.

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