This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add some missing strict FP vector lowering
ClosedPublic

Authored by john.brawn on Jan 20 2022, 8:48 AM.

Details

Summary

Also add a test for the codegen of strict FP vector operations so these changes get tested.

Diff Detail

Event Timeline

john.brawn created this revision.Jan 20 2022, 8:48 AM
john.brawn requested review of this revision.Jan 20 2022, 8:48 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 20 2022, 8:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Matt added a subscriber: Matt.Jan 20 2022, 8:59 AM

Is it possible to break the 4 subtasks into separate reviews?

-In clang generate fcmps when appropriate for neon intrinsics
-Fix legalization of scalarized strict FP vector operations
-Add some missing strict FP handling to AArch64TargetLowering
-Adjust the aarch64-neon-intrinsics-constrained.c clang test to expect the right output and un-XFAIL it.
john.brawn retitled this revision from [AArch64] Fixes for strict FP vector instructions to [AArch64] Add some missing strict FP vector lowering.
john.brawn edited the summary of this revision. (Show Details)

The other parts of this have been split off into D118257, D118258, and D118259. I've also added some extra testing, as the clang test aarch64-neon-intrinsics-constrained wasn't actually testing all of the changes here.

dmgreen added inline comments.Feb 1 2022, 2:17 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1492

Can you split this into a separate patch? I know I sound like a broken record, but it doesn't seem to be related to the converts below.

Also pre-committing as much of the test that works as possible would cut it down from this patch quite a bit.

3431

Some formatting apparently needs fixing up.

3682

Op.getOperand(1) -> In?

3703

dl is already defined.

3706

Op.getOperand(IsStrict ? 1 : 0) -> In?

3709

This isn't used anywhere

john.brawn added inline comments.Feb 1 2022, 9:45 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1492

Instead of a separate patch just for these two, it would probably make more sense to move them into D114946 with the rest of the setOperationAction lines.

On the test, without the changes in this patch it hits an assertion failure so as a separate commit before this it wouldn't be able to test anything.

dmgreen added inline comments.Feb 1 2022, 1:07 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1492

D114946 is already pretty big :)

What about the add_v4f32 test (for example) requires the changes in this patch? That's what I meant by "precommit as much as possible".

My other question about this was going to be - why can't we use the vector instructions for STRICT_FSETCCS? The FCMGE and FCMGT look like they would set exception flags to me, but I may be misunderstanding some part of it.

john.brawn planned changes to this revision.Feb 2 2022, 3:08 AM
john.brawn added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1492

D114946 is already pretty big :)

A bit, though only in terms of number of lines. Conceptually it fairly simple, as it's almost all "strict_xyz can be handled the same as (non-strict) xyz".

What about the add_v4f32 test (for example) requires the changes in this patch? That's what I meant by "precommit as much as possible".

I _could_ try and disentangle the parts that compile without this patch, but it seems like more work than it's worth.

My other question about this was going to be - why can't we use the vector instructions for STRICT_FSETCCS? The FCMGE and FCMGT look like they would set exception flags to me, but I may be misunderstanding some part of it.

Hmm, it looks like the FCMXY instructions are inconsistent in their handling of NaNs. FCMEQ performs a quiet comparison (only signalling NaNs raise an exception) like FCMP and STRICT_FSETCC, FCMGT etc. performs a signalling comparison (all NaNs raise an exception) like FCMPE and STRICT_FSETCCS. I'll adjust the comments (and also move this to D114946).

john.brawn updated this revision to Diff 405213.Feb 2 2022, 4:14 AM

Update based on review comments and adjust formatting.

dmgreen accepted this revision.Feb 7 2022, 7:39 AM

Thanks for the changes. LGTM

This revision is now accepted and ready to land.Feb 7 2022, 7:39 AM
This revision was landed with ongoing or failed builds.Feb 17 2022, 8:11 AM
This revision was automatically updated to reflect the committed changes.