This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Make fullfp16 instructions not conditionalisable.
ClosedPublic

Authored by simon_tatham on Feb 6 2019, 8:46 AM.

Details

Summary

More or less all the instructions defined in the v8.2a full-fp16
extension are defined as UNPREDICTABLE if you put them in an IT block
(Thumb) or use with any condition other than AL (ARM). LLVM didn't
know that, and was happy to conditionalise them.

In order to force these instructions to count as not predicable, I had
to make a small Tablegen change. The code generation back end mostly
decides if an instruction was predicable by looking for something it
can identify as a predicate operand; there's an isPredicable bit flag
that overrides that check in the positive direction, but nothing that
overrides it in the negative direction.

(I considered the alternative approach of actually removing the
predicate operand from those instructions, but thought that it would
be more painful overall for instructions differing only in data type
to have different shapes of operand list. This way, the only code that
has to notice the difference is the if-converter.)

So I've added an isUnpredicable bit alongside isPredicable, and set
that bit on the right subset of FP16 instructions, and also on the
VSEL, VMAXNM/VMINNM and VRINT[ANPM] families which should be
unpredicable for all data types.

I've included a couple of representative regression tests, both of
which previously caused an fp16 instruction to be conditionalised in
ARM state and (with -arm-no-restrict-it) to be put in an IT block in
Thumb.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.Feb 6 2019, 8:46 AM

It's sort of weird to have an instruction that has a predicate operand, yet can't be predicated... but I guess if that's the way the ISA is constructed, we should respect it.

Do we have a check in the assembler for this sort of invalid instruction?

Good question. Testing it with llvm-mc, what seems to happen (with this patch applied) is that if I write one of these instructions after an explicit it instruction then I get "error: instructions in IT block must be predicable", but if there's no preceding IT then it's accepted without complaint.

That probably isn't what I wanted – but it's hard to revert by adding a check in validateInstruction, because a side effect of this change is that now MCID.findFirstPredOperandIdx() returns -1 for those instructions, so I can't find the operand that's already had an unacceptable condition put into it. Hmmm.

Added an error check in ARMAsmParser and a test that exercises it.

Could I ping this, please? I've added the assembler check @efriedma asked about; is there anything else I need to change?

Do we need special handling for fp16 NEON instructions in Thumb mode? I think the compiler won't predicate them anyway (it's deprecated, and some CPUs have known errata; see ARMBaseInstrInfo::isPredicable), but it might affect the assembler.

Does the disassembler also need any special handling here? I guess that's less of a priority, but maybe something to look into as a followup.

llvm/test/CodeGen/ARM/fp16-no-condition.ll
27 ↗(On Diff #185979)

Should explicitly check for vcmp.f16. And we should probably have a corresponding test for vcmpne.f32 to make sure this is actually testing what it's supposed to test. (I think the compiler already refuses to predicate NEON instructions because it's deprecated, but I guess that doesn't apply here.)

I'm not as familiar with the NEON FP16 instructions, but in a quick
look I didn't see a corresponding issue with predicating them – e.g.
the vector vadd.f32 and vadd.f16 are just as predicable as each
other, unlike the scalar ones.

Here's a revised patch in which I've added checks at disassembly time,
and a couple of tests for them.

I've made the disassembler return SoftFail instead of Fail when it
sees a predicated fp16 instruction (reflecting that the ArmARM defines
them as CONSTRAINED UNPREDICTABLE rather than actually undefined or
meaning something else), and I've only added a token test or two to
make sure the mechanism works (rationale: the other set of tests has
exhaustively checked that the Unpredicable flag is set everywhere it
should be, and this one just needs to check that it's correctly
handled in disassembly).

simon_tatham marked an inline comment as done.Feb 22 2019, 9:10 AM
This revision is now accepted and ready to land.Feb 22 2019, 12:54 PM
This revision was automatically updated to reflect the committed changes.