This is an archive of the discontinued LLVM Phabricator instance.

ARM: Enable DP copy, load and store instructions for FPv4-SP
ClosedPublic

Authored by olista01 on Aug 14 2014, 8:53 AM.

Details

Summary

The FPv4-SP floating-point unit is generally reffered to as single-precision only, but it does have double-precision registers and load, store and GPR<->DPR move instructions which operate on them. This patch enables the use of these registers, the main advantage of which is that we now comply with the AAPCS-VFP calling convention. This partially reverts r209650, which added some AAPCS-VFP support, but did not handle return values or alignment of double arguments in registers.

This patch also adds tests for Thumb2 code generation for floating-point instructions and intrinsics, which previously only existed for ARM.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 12511.Aug 14 2014, 8:53 AM
olista01 retitled this revision from to ARM: Enable DP copy, load and store instructions for FPv4-SP.
olista01 updated this object.
olista01 edited the test plan for this revision. (Show Details)
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: Unknown Object (MLST).

Hi Oliver,

This seems to be lots of separate patches lumped together. I started enumerating them but lost count (ABI, VMOV, CMOV, Predicates, at least 1 libcall issue, ...). Please could you split them up?

Thanks.

Tim.

Hi Oliver,

Overall a good patch. I have a few comments inline and I also agree with Tim that this could actually be lots of smaller patches.

Feel free to address the comments here, but apply them (where applicable) to the individual patches later on.

cheers,
--renato

lib/Target/ARM/ARMCallingConv.h
238

Wild guess here, but shouldn't this be something like std::min(Size, 8)?

lib/Target/ARM/ARMISelLowering.cpp
3311

Is this the only such assert, or just the one that you hit during tests?

3535

Would this ever be called by v2f64 types?

8636

This looks like left over.

test/CodeGen/Thumb2/float-cmp.ll
4

You could have used the same HARD+SP/DP you used in the test above to simplify this test.

I could split the patch up, but it would not be testable until the last patch, which would remove the condition at about lib/Target/ARM/ARMISelLowering.cpp:411, which makes f64 a legal type. If I committed this patch first, this would introduce regressions until the last patch is applied. Is this still worth doing?

Now looking again, I also can't see how you could separate much, since it all depends on the new flag.

I'd be ok with having it as one patch.

cheers,
--renato

lib/Target/ARM/ARMCallingConv.h
182

Isn't this an independent fix?

olista01 added inline comments.Aug 21 2014, 1:45 AM
lib/Target/ARM/ARMCallingConv.h
182

This is part of the revert of http://reviews.llvm.org/rL209650. r209650 used this mechanism to pass a double as 2 i32s, meaning that an HFA of 4 doubles would have 8 members. Doubles will now show up here as f64s, so the assertion can be tightened up again.

lib/Target/ARM/ARMISelLowering.cpp
3311

The only other legal floating-point type for FPv4-SP is f32, which can be handled by this function. Vector types are only legal for NEON.

3535

No, because v2f64 is not a legal type when isFPOnlySP, and this is only called while legalizing operations, which happens after legalizing types.

olista01 updated this revision to Diff 12750.Aug 21 2014, 1:45 AM
rengolin added inline comments.Aug 21 2014, 2:04 AM
test/CodeGen/ARM/aapcs-hfa-code.ll
79

A bit fragile this change, no? Shouldn't this also be a DAG check?

test/CodeGen/Thumb2/float-ops.ll
4

This one still has some check redundancy. Sorry for being picky, but I fear people will later change checks without properly making sure it makes sense for both DP and SP.

olista01 added inline comments.Aug 21 2014, 3:07 AM
test/CodeGen/ARM/aapcs-hfa-code.ll
79

I'm not sure how this can be done. If I was to make the movs lines DAG checks, they could match in either order, but ONEHI and ONELO would be swapped, so the movt and strd lines would fail to match. Making any more checks DAGs would cause this test to match even if the registers were in the wrong order (swapping ONEHI and ONELO in the strd is a plausible failure mode).

rengolin added inline comments.Aug 21 2014, 4:22 AM
test/CodeGen/ARM/aapcs-hfa-code.ll
79

That's a good point. I wanted to add a CHECK-OR so that all sequential CHECKs could be matched and you'd be able to do:

; CHECK-M4F: strd [[ONELO]], [[ONEHI]], [sp]
; CHECK-M4F-OR: strd [[ONEHI]], [[ONELO]], [sp]

But that's for another commit. :)

olista01 updated this revision to Diff 12756.Aug 21 2014, 4:36 AM

Reduce redundancy of a test

rengolin edited edge metadata.Aug 21 2014, 4:51 AM

Thanks! LGTM.

--renato

t.p.northover edited edge metadata.Aug 21 2014, 4:53 AM

A common way to get around those issues is to add a temporary option, -arm-enable-dp-sp or whatever, which is off by default while the code is still buggy but can be enabled in tests.

It seems slightly excessive in this case if we're just going to remove it again a few days later, but it would definitely make revision control simpler if anyone does ever need to "git blame" the situation. And it'd make each individual patch easier to review and pair up with its tests (though I'm going to be away for the next week anyway, so if Renato doesn't mind...).

Cheers.

Tim.

rengolin accepted this revision.Aug 21 2014, 4:57 AM
rengolin edited edge metadata.

Yeah, I think it's a bit too much in this case, as the patch is not *that* big and does follow a similar line. I think the nightmare of reverting multiple patches would be worse if we split.

cheers,
--renato

This revision is now accepted and ready to land.Aug 21 2014, 4:57 AM

I'm OK with that in this case, but I think in future we should aim for incremental development *on trunk*. This situation didn't have to come up in the first place.

Cheers.

Tim.

Thanks, committed revision 216172.

olista01 closed this revision.Aug 21 2014, 6:02 AM