Page MenuHomePhabricator

[FPEnv][AArch64] Add lowering and instruction selection for STRICT_FP_ROUND
ClosedPublic

Authored by john.brawn on Jan 22 2020, 7:50 AM.

Details

Summary

This gets selected to the appropriate fcvt instruction. Handling from there on isn't fully correct yet, as we need to model fcvt reading and writing to fpsr and fpcr.

Diff Detail

Event Timeline

john.brawn created this revision.Jan 22 2020, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 7:50 AM

Why is this problem unique to STRICT_FP_ROUND? What makes FP_ROUND work? I suspect this is really a missing setOperationAction line for STRICT_FP_ROUND in the AArch64ISelLowering.cpp

Why is this problem unique to STRICT_FP_ROUND? What makes FP_ROUND work? I suspect this is really a missing setOperationAction line for STRICT_FP_ROUND in the AArch64ISelLowering.cpp

It looks like FP_ROUND doesn't work either. If I make AArch64TargetLowering do setOperationAction(ISD::FP_ROUND, MVT::f32, Expand) then I get the same assertion failure ("Invalid TRUNCATE!" at SelectionDAG.cpp:4661). It looks like currently there's no targets that have FP_ROUND set to Expand for scalar floating-point types, so I guess that the FP_ROUND code is never used.

Why is this problem unique to STRICT_FP_ROUND? What makes FP_ROUND work? I suspect this is really a missing setOperationAction line for STRICT_FP_ROUND in the AArch64ISelLowering.cpp

It looks like FP_ROUND doesn't work either. If I make AArch64TargetLowering do setOperationAction(ISD::FP_ROUND, MVT::f32, Expand) then I get the same assertion failure ("Invalid TRUNCATE!" at SelectionDAG.cpp:4661). It looks like currently there's no targets that have FP_ROUND set to Expand for scalar floating-point types, so I guess that the FP_ROUND code is never used.

So does AArch64 set FP_ROUND to Legal or Custom? STRICT_FP_ROUND should do the same.

So does AArch64 set FP_ROUND to Legal or Custom? STRICT_FP_ROUND should do the same.

FP_ROUND is Custom in AArch64. I could add something to the AArch64 target to handle this, but it's a bit odd that the default behaviour is for an invalid operation to be generated (and I see that -mtriple=sparc hits the same assertion failure, as it also has FP_ROUND set to Custom).

So does AArch64 set FP_ROUND to Legal or Custom? STRICT_FP_ROUND should do the same.

FP_ROUND is Custom in AArch64. I could add something to the AArch64 target to handle this, but it's a bit odd that the default behaviour is for an invalid operation to be generated (and I see that -mtriple=sparc hits the same assertion failure, as it also has FP_ROUND set to Custom).

STRICT_ contains a lot of hacks to try to guess what to do by trying to see what the target does for non-strict nodes. That's why there's a call to getStrictFPOperationAction which will query the non-strict behavior. But it doesn't handle Custom well because we don't know what to do with it.

The change you've proposed here will end up causing the STRICT_FP_ROUND node to be considered Legal even though its set to Expand. It will then be mutated to the non-strict node during isel which is technically wrong. And in this case it works because there are isel patterns for most of the FP_ROUND combinations since the Custom handler only does something special for fp128. So fp128 STRICT_FP_ROUND will end up failing isel since it really needs to be Custom handled.

We want to remove all the mutation code once the in tree targets all support STRICT_ nodes properly. It's a hack to enable targets to limp along a little bit, but in hindsight may have been a bad idea since it creates the illusion of things working when they really don't.

kpn added a subscriber: kpn.Jan 22 2020, 10:57 AM
kpn added a comment.Jan 22 2020, 11:02 AM

We want to remove all the mutation code once the in tree targets all support STRICT_ nodes properly. It's a hack to enable targets to limp along a little bit, but in hindsight may have been a bad idea since it creates the illusion of things working when they really don't.

It may be worthwhile to remove the mutation code sooner. For ops that are Custom we can't even pretend that things work. And a loud failure up front is better than generating code that silently doesn't do what it was asked to do. Plus, removing the mutation code makes it simpler to read and can avoid confusion from people who haven't been following it for years.

john.brawn retitled this revision from [FPEnv][AArch64] Don't expand STRICT_FP_ROUND to an illegal truncating store to [FPEnv][AArch64] Add lowering and instruction selection for STRICT_FP_ROUND.
john.brawn edited the summary of this revision. (Show Details)
john.brawn added reviewers: dmgreen, t.p.northover.

Changed to add handling in the aarch64 backend.

This looks good to me from a strict FP perspective, but maybe wait a bit for someone more familiar with AArch64 to take a look.

dmgreen accepted this revision.Jan 30 2020, 3:57 AM

LGTM

This revision is now accepted and ready to land.Jan 30 2020, 3:57 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Feb 17 2020, 2:33 AM

A user reported running into asserts when compiling musl for aarch64 with -frounding-math.
It appears to be fixed (well, at least it doesn't assert..) by this patch. Should it be cherry-picked to the 10.x branch?

A user reported running into asserts when compiling musl for aarch64 with -frounding-math.
It appears to be fixed (well, at least it doesn't assert..) by this patch. Should it be cherry-picked to the 10.x branch?

There's a bunch of strict-math-related commits that are present on trunk that aren't present on 10.x and which should be cherry-picked. The complete list is, I think:

594a89f7270d [FPEnv][ARM] Don't call mutateStrictFPToFP when lowering
8bc790f9e6a6 [AArch64][FPenv] Update chain of int to fp conversion
0ec57972967d [ARM] Fix infinite loop when lowering STRICT_FP_EXTEND
68cf574857c8 [FPEnv][AArch64] Add lowering of f128 STRICT_FSETCC
b37d59353f69 [FPEnv][ARM] Add lowering of STRICT_FSETCC and STRICT_FSETCCS
0bb9a27c9895 [FPEnv][AArch64] Add lowering and instruction selection for strict conversions
258d8dd76afd [FPEnv][AArch64] Add lowering and instruction selection for STRICT_FP_ROUND
2224407ef5ba Add lowering of STRICT_FSETCC and STRICT_FSETCCS

hans added a comment.Feb 18 2020, 7:56 AM

There's a bunch of strict-math-related commits that are present on trunk that aren't present on 10.x and which should be cherry-picked. The complete list is, I think:

Thanks! I've pushed those to 10.x in f87a0929c6bd59750e424d06581507cdfd439a56..f636e9feb9f0969e3b563d3140db5a0faa1e30d8

Please let me know if you think of more commits that should be picked over.