This is an archive of the discontinued LLVM Phabricator instance.

ARM: add support for QBIT
Needs RevisionPublic

Authored by compnerd on Nov 3 2014, 9:15 PM.

Details

Reviewers
t.p.northover
Summary

Add support for the __ARM_FEATURE_QBIT preprocessor macro. Also add associated
intrinsics for checking the state of the Q flag. This is supported on ARMv5
variant 'e' and later CPUs. These implement section 6.4.6 and 9.1.1 of the
ACLE.

Diff Detail

Event Timeline

compnerd updated this revision to Diff 15746.Nov 3 2014, 9:15 PM
compnerd retitled this revision from to ARM: add support for QBIT.
compnerd updated this object.
compnerd edited the test plan for this revision. (Show Details)
compnerd added a subscriber: Unknown Object (MLST).
kongyi requested changes to this revision.Nov 4 2014, 9:49 AM
kongyi edited edge metadata.

Hi Saleem,

The patch is missing unit tests for the ACLE header.

Thanks,
Yi

lib/Headers/arm_acle.h
110

According to ACLE 2.0:

... intrinsics are available when __ARM_FEATURE_QBIT is defined

Please guard these functions with the corresponding flag.

This revision now requires changes to proceed.Nov 4 2014, 9:49 AM
t.p.northover edited edge metadata.Nov 4 2014, 12:40 PM

Hi Saleem,

I don't think these are quite right yet...

lib/Headers/arm_acle.h
116

I think ACLE says this function should return 1 when the flag is set.

121

This also doesn't seem to do what ACLE says: set the Q flag based on the LSB of the input.

Also, I don't think "cpsr_q" exists, does it? Both the v7A and v7M descriptions I've found lump it in with NZCV (i.e. "cpsr_nzcvq"). Which probably means the assembly should be marked as clobbering CPSR too.

I'm also a bit worried about how these intrinsics will work on v5 cores. Perhaps __ARM_FEATURE_QBIT should only be defined in ARM mode there?

Cheers.

Tim.

rengolin added inline comments.Nov 4 2014, 1:20 PM
lib/Headers/arm_acle.h
121

I may be wrong, but I think you need to mrs first to get the bit pattern, change this value based on (occurred != 0) shifted to the right place and writing it back with msr, or you'll end up writing zeroes to the other flags. Or is that only the case with registers?

125

This will probably need some builtin/intrinsic, since, if I read the ACLE correctly, the context is from *here* to the end of the scope. But it can be left for another set of patches, obviously.

t.p.northover added inline comments.Nov 4 2014, 1:35 PM
lib/Headers/arm_acle.h
121

That would preserve NZCV and allow LLVM more optimisation freedom, but at the cost of a more expensive __set_saturation_occurred.

As long as LLVM's aware that we *have* clobbered NZCV (slightly confusingly tracked as CPSR in the 32-bit backend) it should work either way.

Of course, we don't actually model Q now, so we're not making the required guarantees anyway (I think LLVM would consider itself free to move a QADD past these no matter what we do, for example).

rengolin added inline comments.Nov 4 2014, 1:48 PM
lib/Headers/arm_acle.h
121

Shouldn't we be fixing that in LLVM first, then?

rengolin resigned from this revision.May 8 2015, 5:22 AM
rengolin removed a reviewer: rengolin.
kongyi resigned from this revision.Mar 20 2017, 1:39 PM