This is an archive of the discontinued LLVM Phabricator instance.

Differential D88062

[ARM] add .f16 -> .16 token alias
AbandonedPublic

Authored by ychen on Sep 21 2020, 6:30 PM.

Details

Reviewers
t.p.northover
SjoerdMeijer
rengolin
ostannard
Summary

Otherise asm instrutions like "vst1.f16" are not parsable.
I'm not sure if 2cf294a213c01d074c35f1a6d579092da8caa54c should have include
this or not and googling for the table number does not reveal anything for me.
Is this right?

Diff Detail

Unit TestsFailed

TimeTest
30 mslinux > LLVM.MC/ARM::fullfp16-neg.s
100 mslinux > LLVM.MC/ARM::mve-fp-registers.s
90 mslinux > LLVM.MC/ARM::thumbv8.1m.s
40 mslinux > LLVM.MC/ARM::vmovhr.s

Event Timeline

ychen created this revision.Sep 21 2020, 6:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2020, 6:30 PM
Herald added subscribers: llvm-commits, danielkiss, hiraditya, kristof.beyls. · View Herald Transcript
ychen requested review of this revision.Sep 21 2020, 6:30 PM
Harbormaster completed remote builds in B72454: Diff 293310.Sep 21 2020, 7:06 PM
dmgreen added a subscriber: dmgreen.Sep 21 2020, 11:53 PM

Hello. Can you add some tests? Perhaps something to places like neon-vst-encoding.s/neon-vld-encoding.s

dmgreen added a comment.Sep 21 2020, 11:54 PM

Is this right?

GCC does this, so it seems like a fine idea to me.

ychen retitled this revision from [ARM] add .f16 -> .f16 token alias to [ARM] add .f16 -> .16 token alias.Sep 22 2020, 1:21 PM
ychen added a comment.Sep 24 2020, 9:48 PM

This patch caused some failures (fullfp16-neg.s etc.) and seems changed the behavior of parsing "vmov.f16 s1,r2" for -fp16 from fail to pass. With this patch and -fp16, "vmov.f16 s1,r2" would be parsed to "vmov.16 s1,r2" which is defined to alias "vmov s1,r2".

2cf294a213c01d074c35f1a6d579092da8caa54c implemented "Data type specification flexibility" (in ArmV8-A manual, paragraph F2.9.5, Table F2-9) except f16, so adding .f16 to .16 case here should be doing the right thing. IIUC "Data type specification flexibility" should be restricted to <dt> field which 2cf294a213c01d074c35f1a6d579092da8caa54c did not implement (judging from the failures caused by this patch). It has been almost 11 years, so I guess the feature is not widely used. If I'm going to make TokenAlias restricted to <dt> field as the manual says, "vst1.f16" is still not parsable beause it does not use <dt> field.

"vst1.f16" -> "vst1.16" seems be a GCC extension. GNU as docs does not mention these. It is hard to implement this GCC-compatible function without looking into GNU as source code.

dmgreen added a reviewer: ostannard.Sep 25 2020, 6:00 AM

Hmm...

2cf294a213c01d074c35f1a6d579092da8caa54c possibly pre-dates when fp16 was a common thing in the ARM backend.

vmov.16 r3, s6 (or vmov.p8 r5, s2) seems like an odd instruction to me if they are aliases of vmov, but we are reaching the limit of my very narrow assembler knowledge. It seems that gcc will treat vmov.16 as a vmov.f16 (and require fullfp16). It will not accept vmov.p8, but other assemblers will and treat them like a vmov.

Is this to ease porting between gcc and clang?

ychen added a comment.Sep 25 2020, 8:44 AM
In D88062#2294780, @dmgreen wrote:

Is this to ease porting between gcc and clang?

Yeah, sort of. There are not many uses of vst1.f16, so I could replace them with vst1.16 in the source code directly.

ychen abandoned this revision.Nov 10 2020, 10:20 AM

Abandoning. It doesn't feel like a trivial fix and I could workaround this offline.

Revision Contents

PathSize
llvm/
lib/
Target/
ARM/
1 line

Diff 293310

llvm/lib/Target/ARM/ARMInstrFormats.td

Show First 20 Lines • Show All 2,766 Lines • ▼ Show 20 Lines
def : TokenAlias<".i8", ".8">; def : TokenAlias<".i8", ".8">;
def : TokenAlias<".i16", ".16">; def : TokenAlias<".i16", ".16">;
def : TokenAlias<".i32", ".32">; def : TokenAlias<".i32", ".32">;
def : TokenAlias<".i64", ".64">; def : TokenAlias<".i64", ".64">;
def : TokenAlias<".p8", ".8">; def : TokenAlias<".p8", ".8">;
def : TokenAlias<".p16", ".16">; def : TokenAlias<".p16", ".16">;
def : TokenAlias<".f16", ".16">;
def : TokenAlias<".f32", ".32">; def : TokenAlias<".f32", ".32">;
def : TokenAlias<".f64", ".64">; def : TokenAlias<".f64", ".64">;
def : TokenAlias<".f", ".f32">; def : TokenAlias<".f", ".f32">;
def : TokenAlias<".d", ".f64">; def : TokenAlias<".d", ".f64">;