Page MenuHomePhabricator

ARM: Implement big endian bit-conversion for NEON types
ClosedPublic

Authored by cpirker on May 7 2014, 7:48 AM.

Details

Reviewers
jmolloy
Summary

Hi All,

This patch enables correct conversion between integer and vector data in big endian mode (data format differs wrt. endian mode).
The patch covers the conversion between floating point and vector types, and the function argument passing of vector types providing compatibility to the ARM ABI.
Big endian bit-conversions are implemented using VREV64 and VREV32 instructions.

Thanks,
Christian

Diff Detail

Event Timeline

cpirker updated this revision to Diff 9173.May 7 2014, 7:48 AM
cpirker retitled this revision from to ARM: Implement big endian bit-conversion for NEON types.
cpirker updated this object.
cpirker edited the test plan for this revision. (Show Details)
cpirker added a subscriber: Unknown Object (MLST).
cpirker added a subscriber: Konrad.May 7 2014, 8:17 AM
jmolloy added a subscriber: jmolloy.May 7 2014, 8:24 AM

Hi Christian,

Generally this looks good and I have no real problems with it, just a few small nits.

The patch covers the conversion between floating point and vector types, and the function argument passing of vector types providing compatibility to the ARM ABI.

It doesn't look like this patch does anything for argument passing or lowering formal arguments. Is this going to follow in another patch or do you think there's nothing to do here?

Cheers,

James

lib/Target/ARM/ARMFastISel.cpp
192

This function seems unused?

lib/Target/ARM/ARMISelLowering.cpp
3827

This seems too complex for a ternary - I'd prefer to see it written as an if/else.

lib/Target/ARM/ARMInstrNEON.td
2369

Why are these not allowed in big-endian mode? and why only these patterns?

6261

These look fine, but what was your methodology for generating them? Did you do them all by hand, or use some script?

Hi James,

Thanks for your feedback.

It doesn't look like this patch does anything for argument passing or lowering formal arguments. Is this going to follow in another patch or do you think there's nothing to do here?

The patch converts vector arguments into integer format (and vice versa) when passing vectors as function arguments, as required by the ABI.

Thanks,
Christian

lib/Target/ARM/ARMFastISel.cpp
192

This function is required to evaluate the [IsBE] predicate for the bitconversion rules that specify the VREV instructions.

lib/Target/ARM/ARMISelLowering.cpp
3827

Yes, if/else would be more readable.

lib/Target/ARM/ARMInstrNEON.td
2369

v2f64 is not compatible with v4i32 in big endian mode (a vrev instruction would be needed). All other patterns like v8i8,... are already disabled in big endian mode. Not sure why this specific rule was left active for both endian modes.

6261

I did it by hand, based on similar patch for AArch64 (D3424).

jmolloy edited edge metadata.May 7 2014, 10:16 AM

Hi Christian,

The patch converts vector arguments into integer format (and vice versa) when passing vectors as function arguments, as required by the ABI.

Whereabouts, exactly? I see no changes to the calling convention .td or the calling convention parts of ISel lowering.

Cheers,

James

lib/Target/ARM/ARMFastISel.cpp
192

But that predicate isn't defined anywhere in this patch.

lib/Target/ARM/ARMInstrNEON.td
2369

Ah yes, I see. Thanks.

Hi James,

Whereabouts, exactly? I see no changes to the calling convention .td or the calling convention parts of ISel lowering.

These conversions are done implicitly when fitting a vector value into a GPR(s), therefore no changes in the calling convetions .td.

Thanks,
Christian

lib/Target/ARM/ARMFastISel.cpp
192

The [IsBE] predicate is defined in ARMInstrInfo.td (see line 305).

These conversions are done implicitly when fitting a vector value into a GPR(s), therefore no changes in the calling convetions .td.

But vectors don't get implicitly moved to GPRs unless you're compiling soft-float. Are you only testing softfloat, not hardfloat?

Cheers,

James

lib/Target/ARM/ARMFastISel.cpp
192

I see. Strange that this hasn't been required before now - is it that you haven't tested with FastISel before this patch?

The * should go nearest the function name: const TargetLowering *getTargetLowering().

Hi James,

But vectors don't get implicitly moved to GPRs unless you're compiling soft-float. Are you only testing softfloat, not hardfloat?

All testing includes both soft and hard-float mode.

Thanks,
Christian

cpirker updated this revision to Diff 9183.May 7 2014, 12:02 PM
cpirker edited edge metadata.

Hi James,

I updated the patch:

  • "*" nearest the function name in "lib/Target/ARM/ARMFastISel.cpp"
  • Use i/else in "lib/Target/ARM/ARMISelLowering.cpp"

Thanks,
Christian

Hi James,

The mentioned ABI related vector conversions are applicable in soft-mode only.
In hard-mode vectors are passed using the vector registers. In such a case these conversions are triggered by BITCAST operations only.

Thanks,
Christian

cpirker updated this revision to Diff 9257.May 9 2014, 6:51 AM

Hi all,

I updated the patch with two updated test files.

Thanks,
Christian

Hi Christian,

Sorry, must have missed your comment first time round.

In hard-mode vectors are passed using the vector registers. In such a case these conversions are triggered by BITCAST operations only.

Indeed - this is what I was getting at. So where are those BITCASTs inserted in your patch? They aren't, and I don't think your patch will work in hard float mode. So do you want to make a followon patch to work for hard-float mode, or do you want me to treat this patch as working for hard-float mode as well (as you mentioned in an earlier comment)?

Cheers,
James

Hi James,

Indeed - this is what I was getting at. So where are those BITCASTs inserted in your patch? They aren't, and I don't think your patch will work in hard float mode. So do you want to make a followon patch to work for hard-float mode, or do you want me to treat this patch as working for hard-float mode as well (as you mentioned in an earlier comment)?

The "bitconvert" patterns in this patch are valid for both soft and hard float modes.
However in hard float there will be less integer to vector and vice versa converts as generated by LLVM.

Thanks,
Christian

Hi Christian,

This is still not what I'm getting at. The A32 ABI says that vectors passed over function call boundaries in vector registers (which happens in hard float mode) must be passed in a form as if they were loaded by a VLDM. From the ABI:

"""A 128-bit containerized vector type is passed as if it were loaded from its

memory format into a 128-bit vector register (Qn) with a single VLDM of the
two component 64-bit vector registers (for example, VLDM r0,{d2,d3} would
load q1)"""

Your patch does not address this. Vectors will be passed with incompatible types across ABI boundaries.

James

Hi James,

you are right, vector passing in VLDM format is not addressed by this patch.

Thanks,
Christian

Hi Christian,

OK, we're finally on the same page :) So do you want to address this as part of this patch, or leave it for a followup?

Cheers,

James

Hi James,

I will do this in another patch.

Thanks,
Christian

Hi James,

I checked the following testcase:

llc -march armeb -mtriple=arm-eabi -mattr v7,neon -float-abi=hard

with:

define void @test( <4 x i32> %var, <4 x i32>* %store ) {
  store <4 x i32> %var, <4 x i32>* %store
  ret void
}

The generated code is as follows (both for le and be):

vst1.64 {d0, d1}, [r0:128]
bx lr

I believe that is ABI compliant. please let me know if you think otherwise.

Thanks,
Christian

jmolloy accepted this revision.May 12 2014, 2:28 AM
jmolloy edited edge metadata.

Hi Christian,

OK, the patch looks good as-is. It really needs a hard-float testcase - otherwise someone could change the behaviour that seems to accidentally work for BE and regress BE without us noticing.

I'll leave it up to you whether to change this patch or to do it in a followup, though.

Thanks!

James

This revision is now accepted and ready to land.May 12 2014, 2:28 AM
cpirker closed this revision.May 12 2014, 4:44 AM

Hi James,

I added the hard-float testcase to the test file "test/CodeGen/ARM/big-endian-neon-bitconv.ll".
I committed this patch as rL208538.

Thanks,
Christian

Hi Christian,

Thanks, but I'm afraid that's not sufficient. That testcase has all arguments passed as pointers, which it then loads. It does not test vector argument passing. See for example test/CodeGen/ARM64/big-endian-callee.ll and test/CodeGen/ARM64/big-endian-caller.ll.

Please could you add an equivalent testcase for ARM.

Cheers,

James

-----Original Message-----
From: Christian Pirker [mailto:cpirker@a-bix.com]
Sent: 12 May 2014 12:44
To: cpirker@a-bix.com; James Molloy
Cc: kanheim@a-bix.com; Amara Emerson; llvm-commits@cs.uiuc.edu
Subject: Re: [PATCH] ARM: Implement big endian bit-conversion for NEON
types

Hi James,

I added the hard-float testcase to the test file "test/CodeGen/ARM/big-
endian-neon-bitconv.ll".
I committed this patch as rL208538.

Thanks,
Christian

http://reviews.llvm.org/D3651

Hi Christian,

I've taken a closer look, and it looks like this should actually Just Work (tm).

It turns out all vector types are explicitly bitcast to v2f64 in ARMCallingConv.td:

// Handle all vector types as either f64 or v2f64.
CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,

CCIfType<[v2f64], CCAssignToReg<[Q0, Q1, Q2, Q3]>>,

I'm not sure why it was decided to implement that way (happened in r73095, or approximately the stone age, when Anton landed initial hard float support).

V2f64 is also coincidentally exactly the right type for proper parameter passing on Big Endian, and so yes, this should just work for hardfloat.

I'll re-review the rest of your patch now.

Cheers,

James

-----Original Message-----
From: Christian Pirker [mailto:cpirker@a-bix.com]
Sent: 12 May 2014 09:52
To: cpirker@a-bix.com; James Molloy
Cc: kanheim@a-bix.com; Amara Emerson; llvm-commits@cs.uiuc.edu
Subject: Re: [PATCH] ARM: Implement big endian bit-conversion for NEON
types

Hi James,

I checked the following testcase:

llc -march armeb -mtriple=arm-eabi -mattr v7,neon -float-abi=hard

with:

define void @test( <4 x i32> %var, <4 x i32>* %store ) {
  store <4 x i32> %var, <4 x i32>* %store
  ret void
}

The generated code is as follows (both for le and be):

vst1.64 {d0, d1}, [r0:128]
bx lr

I believe that is ABI compliant. please let me know if you think otherwise.

Thanks,
Christian

http://reviews.llvm.org/D3651

  • IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Hi Christian,

Please don't be confused by this email - I "sent" it much earlier today, as justification for why I accepted your Phab review.

It turns out that my work inbox was full so the email silently didn't send until I'd cleared it out.

Sorry for any confusion,

James

-----Original Message-----
From: James Molloy [mailto:james.molloy@arm.com]
Sent: 12 May 2014 13:26
To: cpirker@a-bix.com; James Molloy
Cc: kakaka@akakaka.com; kanheim@a-bix.com; Amara Emerson; llvm-
commits@cs.uiuc.edu
Subject: Re: [PATCH] ARM: Implement big endian bit-conversion for NEON
types

Hi Christian,

I've taken a closer look, and it looks like this should actually Just Work (tm).

It turns out all vector types are explicitly bitcast to v2f64 in
ARMCallingConv.td:

// Handle all vector types as either f64 or v2f64.
CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,

CCIfType<[v2f64], CCAssignToReg<[Q0, Q1, Q2, Q3]>>,

I'm not sure why it was decided to implement that way (happened in r73095,
or approximately the stone age, when Anton landed initial hard float
support).

V2f64 is also coincidentally exactly the right type for proper parameter
passing on Big Endian, and so yes, this should just work for hardfloat.

I'll re-review the rest of your patch now.

Cheers,

James

> -----Original Message-----
> From: Christian Pirker [mailto:cpirker@a-bix.com]
> Sent: 12 May 2014 09:52
> To: cpirker@a-bix.com; James Molloy
> Cc: kanheim@a-bix.com; Amara Emerson; llvm-commits@cs.uiuc.edu
> Subject: Re: [PATCH] ARM: Implement big endian bit-conversion for NEON
> types
>
> Hi James,
>
> I checked the following testcase:
>
> llc -march armeb -mtriple=arm-eabi -mattr v7,neon -float-abi=hard
>
> with:
>
> define void @test( <4 x i32> %var, <4 x i32>* %store ) {
> store <4 x i32> %var, <4 x i32>* %store
> ret void
> }
>
> The generated code is as follows (both for le and be):
>
> vst1.64 {d0, d1}, [r0:128]
> bx lr
>
> I believe that is ABI compliant. please let me know if you think otherwise.
>
> Thanks,
> Christian
>
> http://reviews.llvm.org/D3651
>
>

  • IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

    ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

    http://reviews.llvm.org/D3651
  • IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Hi James,

the test file "test/CodeGen/ARM/big-endian-neon-bitconv.ll" that you can find in the rL208538 includes three test functions with 128 bit vector arguments tested for hard and soft float modes.

Thanks,
Christian

Hi James,

would you please confirm my comment above, just to be sure being on the same page.
Nevertheless you can find a punch of more test cases with respect to soft and hard float in D3766.

Thanks,
Christian

Hi Christian,

The testcases in this patch were insufficient, but the ones you've subsequently added (callee and caller) are sufficient.

Cheers,

James