<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">Hi Sam,</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">Thanks for the suggestions! I can confirm that this works, so this is indeed the
<br>
</p>
<p style="margin-top:0;margin-bottom:0">most elegant way to separate the FP16 and FullFP16 rules.</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">This was the last piece of the puzzle. I can now abandon my old approach,</p>
<p style="margin-top:0;margin-bottom:0">and will go for the CCState and this tablegen separation approach; thus we get
<br>
</p>
<p style="margin-top:0;margin-bottom:0">most of the legalization for free and it is more robust than custom lowering</p>
<p style="margin-top:0;margin-bottom:0">loads/stores (and some other corner cases).<br>
</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">Thanks,</p>
<p style="margin-top:0;margin-bottom:0">Sjoerd.<br>
</p>
</div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Sam Parker<br>
<b>Sent:</b> 18 January 2018 14:28:45<br>
<b>To:</b> Sjoerd Meijer; Friedman, Eli; Oliver Stannard; llvm-dev@lists.llvm.org<br>
<b>Cc:</b> nd<br>
<b>Subject:</b> Re: [llvm-dev] [RFC] Half-Precision Support in the Arm Backends</font>
<div> </div>
</div>
<style type="text/css" style="display:none">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
</style>
<div dir="ltr">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Helvetica,sans-serif">
<p style="margin-top:0; margin-bottom:0"></p>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px; margin-top:0px; margin-bottom:0px">
Hi Sjoerd,</div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px; margin-top:0px; margin-bottom:0px">
<br>
</div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px; margin-top:0px; margin-bottom:0px">
<font size="3"><span style="font-size:12pt">For ISel, I think having a separate register class will give you less headache. I wondering if you could get away with not touching the instructions descriptions at all, instead defining external pattens for the FullFP16
case, like so:</span></font><br>
</div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px; margin-top:0px; margin-bottom:0px">
<br>
</div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px; margin-top:0px; margin-bottom:0px">
</div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt">def VCVTBHS: ASuI<0b11101, 0b11, 0b0010, 0b01, 0, (outs SPR:$Sd), (ins SPR:$Sm),</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"> IIC_fpCVTSH, "vcvtb", ".f32.f16\t$Sd, $Sm",</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"> []>,</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"> Requires<[HasFP16]>,</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"> Sched<[WriteFPCVT]>;</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"> </span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt">def : FP16Pat<(f16_to_fp GPR:$a),</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"> </span></font> <font face="Courier New,monospace" size="2"><span style="font-size:10pt">(VCVTBHS
(COPY_TO_REGCLASS GPR:$a, SPR))>;</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"> </span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt">def : FullFP16Pat<(f32 (fpextend HPR:$Sm)),</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"> <font face="Courier New,monospace" size="2"><span style="font-size:10pt">(VCVTBHS (COPY_TO_REGLASS HPR:$Sm, SPR)>;</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Courier New,monospace" size="2"><span style="font-size:10pt"><br>
</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Calibri,Helvetica,sans-serif" size="3"><span style="font-size:12pt">I'm not sure of the COPY_TO_REGLASS semantics, but I would (dangerously) assume that it when it comes to copying
the values between registers, it will be noticed that HPR and SPR actually alias each other and so no copy is needed. I hope t</span></font><font face="Calibri,Helvetica,sans-serif" size="3"><span style="font-size:12pt">his approach would allow for a clean
separation of the </span></font><font face="Calibri,Helvetica,sans-serif" size="3"><span style="font-size:12pt">FP16 and FullFP16 implementations and negate the need to manually type </span></font><font face="Calibri,Helvetica,sans-serif" size="3"><span style="font-size:12pt">cast
each register access.</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Calibri,Helvetica,sans-serif" size="3"><span style="font-size:12pt"><br>
</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Calibri,Helvetica,sans-serif" size="3"><span style="font-size:12pt">cheers,</span></font></div>
<div style="font-family:Calibri,Helvetica,sans-serif; font-size:16px"><font face="Calibri,Helvetica,sans-serif" size="3"><span style="font-size:12pt">sam</span></font></div>
<br>
<p></p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<div id="x_Signature">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:rgb(0,0,0); background-color:rgb(255,255,255); font-family:Calibri,Arial,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols">
<p></p>
<p style="font-family:"Times New Roman""><span style="font-family:Calibri,Helvetica,sans-serif">Sam Parker</span></p>
<span style="font-family:Calibri,Helvetica,sans-serif"></span>
<p style="font-family:"Times New Roman""><span style="font-family:Calibri,Helvetica,sans-serif">Compilation Tools Engineer | Arm</span></p>
<span style="font-family:Calibri,Helvetica,sans-serif"></span>
<p style="font-family:"Times New Roman""><span style="font-family:Calibri,Helvetica,sans-serif">. . . . . . . . . . . . . . . . . . . . . . . . . . .</span></p>
<span style="font-family:Calibri,Helvetica,sans-serif"></span>
<p style="font-family:"Times New Roman""><span style="font-family:Calibri,Helvetica,sans-serif">Arm.com</span></p>
<p></p>
</div>
</div>
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Sjoerd Meijer<br>
<b>Sent:</b> 18 January 2018 13:58:29<br>
<b>To:</b> Friedman, Eli; Sjoerd Meijer; Sam Parker; Oliver Stannard; llvm-dev@lists.llvm.org<br>
<b>Cc:</b> nd<br>
<b>Subject:</b> Re: [llvm-dev] [RFC] Half-Precision Support in the Arm Backends</font>
<div> </div>
</div>
<style type="text/css" style="display:none">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
</style>
<div dir="ltr">
<div id="x_x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:rgb(0,0,0); font-family:Calibri,Helvetica,sans-serif,"EmojiFont","Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols">
<p style="margin-top:0; margin-bottom:0"></p>
<div>I would like to revive this thread, as I am struggling a lot with the FP16<br>
implementation in the ARM backend. My implementation in<br>
https://reviews.llvm.org/D38315 is finished (except one case), but a more<br>
robust alternative implementation was suggested. One can indeed argue that my<br>
current implementation is a bit fragile, because it involves manually patching<br>
up the isel dags for a few cases. The suggestion was to look into CCState and<br>
adjusting of the calling convention lowering, inspired by a recent discussion<br>
on the list here: http://lists.llvm.org/pipermail/llvm-dev/2018-January/120098.html.<br>
The benefit of this approach is that I would get most of legalization for free, which is the<br>
fragile bit in my approach at the moment.<br>
Anyway, it's become a long story, but (almost) in chronological order this is<br>
what I've considered. Any hints, tips, suggestions welcome.<br>
<br>
Goal of my exercise:<br>
--------------------<br>
<br>
1) Add match rules for some Armv8.2-A FP16 instructions, e.g.: fsub, fadd,<br>
and also some conversion instructions.<br>
2) Don't regress the "storage-only cases", i.e., when we only have the<br>
conversion instructions available.<br>
<br>
The Approach:<br>
-------------<br>
<br>
1) First, we make f16 legal only when we have FullFP16 support (i.e. when<br>
Armv8.2-A FP16 is supported), so in ARMISelLowering.cpp we add:<br>
<br>
if (Subtarget->hasFullFP16()) {<br>
addRegisterClass(MVT::f16, &ARM::HPRRegClass);<br>
}<br>
<br>
2) This is the first implementation decision, I introduce a new register class<br>
HPR, which is an exact copy of the single-precision register class SPR, except<br>
that it holds f16 types:<br>
<br>
def HPR : RegisterClass<"ARM", [f16], 32, (sequence "S%u", 0, 31)> {<br>
...<br>
<br>
I think this makes sense because half-types sit in the lower 16 bits of the<br>
S-registers, and the reason to create a separate HPR register class is to avoid<br>
typing of all rules that currently use SPR, because if we would allow both f16<br>
and f32 in SPR, then there's a choice and rules need to be typed.<br>
<br>
3) Next, add match rules for some Armv8.2-A FP16 instructions. Instruction<br>
VADDH, which has already a tablegen description but not yet a match rules,<br>
simply becomes:<br>
<br>
def VADDH : AHbI<0b11100, 0b11, 0, 0,<br>
(outs HPR:$Sd), (ins HPR:$Sn, HPR:$Sm),<br>
IIC_fpALU16, "vadd", ".f16\t$Sd, $Sn, $Sm",<br>
[(set HPR:$Sd, (fadd HPR:$Sn, HPR:$Sm))]>, // <~~~ new match rule using HPR<br>
<br>
This is straightforward business so far, but I already learned the hard way<br>
that the conversion are the tricky ones, so I repeat this for an f16 -> f32<br>
upconvert:<br>
<br>
def VCVTBHS: ASuI<0b11101, 0b11, 0b0010, 0b01, 0, (outs SPR:$Sd), (ins HPR:$Sm),<br>
IIC_fpCVTSH, "vcvtb", ".f32.f16\t$Sd, $Sm",<br>
[(set SPR:$Sd, (fpextend HPR:$Sm))]>, // <~~~~ new match rule using HPR and SPR<br>
Requires<[HasFP16]>,<br>
Sched<[WriteFPCVT]>;<br>
<br>
</div>
<p></p>
<p style="margin-top:0; margin-bottom:0"></p>
<div>This new rule matches and fp_extend that consumes an HPR and produces a result<br>
in SPR.<br>
<br>
And that's essentially it and all the code changes I want to make. With these<br>
changes, I want to see an fadd with two f16 opernands being codegen'd, and also<br>
I want to keep the existing tests passing of course.<br>
<br>
The problem:<br>
------------<br>
<br>
The problem appears to be that introducing HPR looks like a good idea, but it<br>
gives problems when FullFP16 is not supported. This is best illustrated with<br>
this existing test which is a simple upconvert of f16 to f32:<br>
<br>
define float @test_extend32(half* %addr) {<br>
%val16 = load half, half* %addr<br>
%val32 = fpext half %val16 to float<br>
ret float %val32<br>
}<br>
<br>
It should generate this code::<br>
<br>
ldrh r0, [r0] ; integer half word load<br>
vmov s0, r0<br>
vcvtb.f32.f16 s0, s0<br>
vmov r0, s0<br>
bx lr<br>
<br>
when we don't have the Armv8.2-A FP16 instructions available, and thus only<br>
have the conversion instructions.<br>
<br>
The problem is in the conversion rules, some rewrite rules to be more specific,<br>
and I think this is one of the culprits:<br>
<br>
def : Pat<(f16_to_fp GPR:$a),<br>
(VCVTBHS (COPY_TO_REGCLASS GPR:$a, SPR))>;<br>
<br>
This rewrite rule is supposed to first move the GPR reg in to a S-registers:<br>
<br>
vmov s0, r0<br>
<br>
and then to the conversion:<br>
<br>
vcvtb.f32.f16 s0, s0<br>
<br>
This rewrite rule gets triggered because the ISEL DAG has indeed this funny<br>
node f16_to_fp (which models this register move and the float-convert):<br>
<br>
t0: ch = EntryToken<br>
t2: i32,ch = CopyFromReg t0, Register:i32 %0<br>
t16: i32,ch = load<LD2[%addr], zext from i16> t0, t2, undef:i32<br>
t12: f32 = fp16_to_fp t16 <~~~~~~~~ FUNNY NODE HERE<br>
t7: i32 = bitcast t12<br>
t9: ch,glue = CopyToReg t0, Register:i32 %r0, t7<br>
t10: ch = ARMISD::RET_FLAG t9, Register:i32 %r0, t9:1<br>
<br>
</div>
<div>And yes, to make it even funnier, this node has an i32 operand, and that's<br>
because we do the half-float load with an integer load instruction.<br>
<br>
And after this rewrite, we end up with this DAG:<br>
<br>
t0: ch = EntryToken<br>
t2: i32,ch = CopyFromReg t0, Register:i32 %0<br>
t16: i32,ch = t2LDRHi12<Mem:LD2[%addr]> t2, TargetConstant:i32<0>, TargetConstant:i32<14>, Register:i32 %noreg, t0<br>
t20: f16 = COPY_TO_REGCLASS t16, TargetConstant:i32<1> <~~~~~~~~~~~~~ PROBLEM HERE<br>
t12: f32 = VCVTBHS t20, TargetConstant:i32<14>, Register:i32 %noreg<br>
t7: i32 = VMOVRS t12, TargetConstant:i32<14>, Register:i32 %noreg<br>
t9: ch,glue = CopyToReg t0, Register:i32 %r0, t7<br>
t10: ch = tBX_RET TargetConstant:i32<14>, Register:i32 %noreg, Register:i32 %r0, t9, t9:1<br>
<br>
It goes all wrong from here, because a f16 value is produced and it is not a<br>
legal type etc.<br>
<br>
The reason that this happens must be because VCVTBHS that is used in<br>
the f16_to_fp rewrite rule, is specified to consume a f16 value, so the<br>
CopyToReg moves from GPR to an S-Registers:<br>
<br>
def : Pat<(f16_to_fp GPR:$a),<br>
(VCVTBHS (COPY_TO_REGCLASS GPR:$a, SPR))>;<br>
<br>
Conclusion:<br>
-----------<br>
<br>
This approach is not going to work:<br>
- because there's rewrite rule X which generates instructions Y that use both<br>
the SPR and HPR descriptions.<br>
- Y can be instruction selected when FullFP16 is enabled, this works and no<br>
problems (and we don't need X).<br>
- but X is used when FullFP16 is not supported, and generates Y that uses HPRs<br>
which is not legal.<br>
<br>
Introducing a new HPR register class is a possibility though, but then special<br>
care needs to be taken in a few cases. This is the first alternative I started<br>
exploring.</div>
<div><br>
</div>
<div>
<div>Alternative 1<br>
--------------<br>
<br>
This is the approach I took in D38315, which made f16 also legal for the<br>
storage-only case, but which I am ready to abandon:<br>
<br>
- Define the HPR register class,<br>
- Use this and make f16 legal also in the storage-only case,<br>
-- This works for the storage-only case, because data processing instruction<br>
that have f16 operands will get promoted and fp_extend and fp_round nodes will<br>
be introduced. Then I manually patch up the f16 loads and stores and a few<br>
other corner cases by manually lowering them. This however is a fragile<br>
approach, which was rightfully commented by Oli, also after being inspired by a<br>
recent discussion about some backends doing their own CCState and function<br>
argument lowering. This is Alternative 3.<br>
-- Another crucial aspect and benefit of this approach, is that this fixes<br>
AAPCS compliance issues: while half-precision arguments sit in single-precision<br>
registers, callees should read (and also write) only the lower 16 bits. I got<br>
this for free by making f16 legal.<br>
<br>
<br>
Alternative 2<br>
-------------<br>
<br>
This alternative aims at getting something ready soon, it relies on<br>
exploring creating one register class and a Clang that generates the<br>
required up and down converts for the storage-only case:<br>
<br>
- We have one SPR registerclass that contains both f16 and f32:<br>
<br>
def SPR : RegisterClass<"ARM", [f16, f32], 32, (sequence "S%u", 0, 31)> {<br>
...<br>
<br>
- Then I will have to type all rules that e.g. use SPR:$a and change it in (f32<br>
SPR:$a):<br>
-- this will be a massive patch,<br>
-- but hopefully will not have a problem in the rewrite rules.<br>
<br>
- Then, I also need to modify a clang workaround, which should inserts the proper up and down<br>
converts for half types that are generated from the usage of _Float16,<br>
similarly like we already do for __fp16. This workaround can then later be replaced<br>
by the CCState implementation.</div>
<div><br>
</div>
<div>
<div>Alternative 3:<br>
----------------<br>
<br>
This is the best approach, where I start looking into the AAPCS compliance<br>
issues first:<br>
<br>
- Fix the AAPCS compliance issues by implementing our own CCState and function<br>
call lowering for f16 arguments.<br>
-- With this we achieve that f16 does not need to be a legal type when in fact<br>
it shouldn't be, so then the "normal" promotions kick in for illegal f16<br>
operands and thus I don't need custom lowering for loads/stores.<br>
<br>
but after that I am unsure how to do the match rules: one or two register<br>
classes. It might be that the difference between Alternative 2 and this one,<br>
is the CCState or Clang workaround. I will investigate a bit more.</div>
<br>
</div>
<br>
<br>
</div>
<br>
<p></p>
<br>
<br>
<div style="color:rgb(0,0,0)">
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_x_divRplyFwdMsg" dir="ltr"><font color="#000000" face="Calibri, sans-serif" style="font-size:11pt"><b>From:</b> llvm-dev <llvm-dev-bounces@lists.llvm.org> on behalf of Sjoerd Meijer via llvm-dev <llvm-dev@lists.llvm.org><br>
<b>Sent:</b> 06 December 2017 08:32<br>
<b>To:</b> Friedman, Eli; llvm-dev@lists.llvm.org<br>
<b>Subject:</b> Re: [llvm-dev] [RFC] Half-Precision Support in the Arm Backends</font>
<div> </div>
</div>
<div dir="ltr">
<div id="x_x_x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:rgb(0,0,0); font-family:Calibri,Helvetica,sans-serif,"EmojiFont","Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols">
<p style="margin-top:0; margin-bottom:0">Thanks a lot for the suggestions! I will look into using vld1/vst1, sounds good.</p>
<p style="margin-top:0; margin-bottom:0">I am custom lowering the bitcasts, that's now the only place where FP_TO_FP16
<br>
</p>
<p style="margin-top:0; margin-bottom:0">and FP16_TO_FP nodes are created to avoid inefficient code generation. I will</p>
<p style="margin-top:0; margin-bottom:0">double check if I can't achieve the same without using these nodes (because I</p>
<p style="margin-top:0; margin-bottom:0">really would like to get completely rid of them).<br>
</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<p style="margin-top:0; margin-bottom:0">Cheers,</p>
<p style="margin-top:0; margin-bottom:0">Sjoerd.<br>
</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<pre>>On 12/4/2017 6:44 AM, Sjoerd Meijer via llvm-dev wrote:
>><i>
</i>>><i> Custom Lowering
</i>><i>> -------------------------
</i>>><i>
</i>>><i> Making f16 legal and not having native load/stores instructions available,
</i>>><i> (no FullFP16 support) means custom lowering loads/stores:
</i>>><i> 1) Since we don't have FP16 load/store instructions available, we create
</i>>><i> integer half-word loads. I unfortunately need the FP16_TO_FP node here,
</i>>><i> because that "models" creating an integer value, which is what we need
</i>>><i> to create a "truncating i16" integer load instructions. Instead, of
</i>>><i> using
</i>>><i> FP16_TO_FP, I have tried BITCASTs, but this can lead to code generation
</i>>><i> to stack loads/stores which I don't want.
</i>>><i> 2) Custom lowering f16 stores is very similar, and creates truncating
</i>>><i> half-word integer stores.
</i>>
>Technically, there are no f16 load/store instructions, yes, but we can
>use NEON vdl1 and vst1 to get something roughly equivalent, right?
>
>You probably want to custom-lower BITCAST instructions; the generic
>sequence emitted by the legalizer is pretty inefficient in most cases.
>
>---
>
>Overall, I think your approach makes sense.
</pre>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>