This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Properly validate the reciprocal estimation
ClosedPublic

Authored by evandro on Jul 12 2016, 8:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

evandro updated this revision to Diff 63685.Jul 12 2016, 8:40 AM
evandro retitled this revision from to [AArch64] Properly validate the reciprocal estimation.
evandro updated this object.
evandro set the repository for this revision to rL LLVM.
t.p.northover edited edge metadata.Jul 12 2016, 8:43 AM

I think you forgot to add llvm-commits as a subscriber (that's what makes it get sent to the list, not the repository). Also tests.

evandro edited edge metadata.Jul 12 2016, 8:44 AM
evandro added a subscriber: llvm-commits.

@t.p.northover,

WRT the tests, I'd appreciate some help. In some cases, lowering would fail because the square root was being expanded into the series for MVT::v4f64, which is obviously not supported by AArch64. I'm at a loss as to why such data type, unsupported by the target, percolated through to the backend.

Would concocting the bytecode using such data type be a sensible test?

On another note, as I'm not very familiar with the latest versions of ARMv8, should FP16 be considered here?

Thank you.

Would concocting the bytecode using such data type be a sensible test?

That's definitely a valid way to write the test if you can get it to crash like that. We should be accepting and legalizing all IR.

On another note, as I'm not very familiar with the latest versions of ARMv8, should FP16 be considered here?

Yes. f16, v4f16 and v8f16 are valid for both if Subtarget->hasFullFP16().

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4579 ↗(On Diff #63685)

Why the extra brackets?

Also, probably v1i64 (a bit of a silly type, but it does exist and should work).

Also, probably v1i64 (a bit of a silly type, but it does exist and should work).

Whaaat? Please, explain.

evandro added a comment.EditedJul 12 2016, 9:21 AM

Sorry, v1f64.

Ah, OK. How about v1f32?

@t.p.northover,

Please, correct me if I'm wrong, but there's neither FRECPE nor FRSQRTE insts in ARMv8.1, are there?

Ah, OK. How about v1f32?

I wouldn't worry about that, I don't think it's used nearly as frequently. The v1f64 issue comes up because that's how arm_neon.h represents some vectors.

Please, correct me if I'm wrong, but there's neither FRECPE nor FRSQRTE insts in ARMv8.1, are there?

That's right. Full support for _Float16 is part of v8.2 I believe, but tracked as a separate feature in LLVM: "fullfp16" if you're doing -mattr.

That's right. Full support for _Float16 is part of v8.2 I believe, but tracked as a separate feature in LLVM: "fullfp16" if you're doing -mattr.

I don't have the v8.2 documentation at hand, so, please indulge me, does it support both FRECPE and FRSQRTE for _Float16?

I don't have the v8.2 documentation at hand, so, please indulge me, does it support both FRECPE and FRSQRTE for _Float16?

It does:

$ echo "frecpe v0.4h, v1.4h" | llvm-mc -triple aarch64 -mattr=+fullfp16 -show-inst
frecpe v0.4h, v1.4h           // <MCInst #859 FRECPEv4f16
                              //  <MCOperand Reg:40>
                              //  <MCOperand Reg:41>>
$ echo "frsqrte v0.4h, v1.4h" | llvm-mc -triple aarch64 -mattr=+fullfp16 -show-inst
frsqrte	v0.4h, v1.4h            // <MCInst #934 FRSQRTEv4f16
                                //  <MCOperand Reg:40>
                                //  <MCOperand Reg:41>>

(plus similar results for scalar operations).

evandro marked an inline comment as done.Jul 12 2016, 1:54 PM
evandro added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4579 ↗(On Diff #63685)

Oops!

evandro updated this revision to Diff 64531.Jul 19 2016, 11:49 AM
evandro marked 2 inline comments as done.

Add validation for the _Float16 types.

evandro updated this revision to Diff 64544.Jul 19 2016, 12:29 PM

Update existing test cases to include the error vector found in the wild.

t.p.northover accepted this revision.Jul 19 2016, 12:50 PM
t.p.northover edited edge metadata.

Thanks Evandro! I think this looks fine now.

This revision is now accepted and ready to land.Jul 19 2016, 12:50 PM
This revision was automatically updated to reflect the committed changes.