This is an archive of the discontinued LLVM Phabricator instance.

[ARM] f16 conversions
ClosedPublic

Authored by SjoerdMeijer on Feb 6 2018, 4:07 AM.

Details

Summary

This is a follow up of r324321, adding f16 <-> f32 and f16 <-> f64
conversion match patterns.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Feb 6 2018, 4:07 AM
olista01 added inline comments.Feb 6 2018, 5:23 AM
lib/Target/ARM/ARMInstrVFP.td
682 ↗(On Diff #132970)

The instruction only requires HasFP16, so why does the pattern need FullFP16? (same question multiple times in this file)

694 ↗(On Diff #132970)

Should this COPY_TO_REGCLASS target HPR, since the output is an f16?

725 ↗(On Diff #132970)

VCVTBHD takes the input in an SPR, so should the COPY_TO_REGCLASS target SPR?

test/CodeGen/ARM/fp16-instructions.ll
237 ↗(On Diff #132970)

It would be better to have each test in its own function, rather then combining them like this.

Addressed comments.

About:

The instruction only requires HasFP16, so why does the pattern need FullFP16? (same question multiple times in this file)

What we are doing here, is "driving" one instruction description by 2 rewrite patterns:
one for FP16, and the other for FullFP16. The pattern names "FullFP16Pat"" and
"FullFP16Pat" are mainly there just to make to make explicit when we are using these
rules. I agree that this is instruction does not FullFP16, so I could remove the predicate
from the pattern definition:

class FullFP16Pat<dag pattern, dag result> : Pat<pattern, result> {
  list<Predicate> Predicates = [HasFullFP16];
}

But as the rewrite rule is using the HPR register class, which is added only when we
have FullFP16 support, I thought it would be good to have it.

olista01 accepted this revision.Feb 6 2018, 8:23 AM

Thanks, LGTM.

This revision is now accepted and ready to land.Feb 6 2018, 8:23 AM
This revision was automatically updated to reflect the committed changes.