[PATCH] D107082: [X86][RFC][WIP] Enable `_Float16` type support on X86 following the psABI

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/docs/ReleaseNotes.rst:136
 
-* ...
+* Support ``half`` type on SSE2 and above targets.
 
----------------
skan wrote:
> Just for curiosity, why is SSE2?
We are following to GCC. The more background about why chosing SSE2 can be found [[ https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg264242.html | here ]]


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:593
 
+    // Half type will be promoted by default.
+    setOperationAction(ISD::FABS, MVT::f16, Promote);
----------------
skan wrote:
> Promote to which type?
If we didn't set the promote type, LLVM will automaticly find the next available type in the same type class as the order defined in MachineValueType.h


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5714
   return VT == MVT::f32 || VT == MVT::f64 || VT.isVector() ||
-         (VT == MVT::f16 && Subtarget.hasFP16());
+         (VT == MVT::f16 && Subtarget.hasBWI());
 }
----------------
skan wrote:
> Add comments for `hasBWI`?
Good question! Taking a look at the caller, this is used to combine integer logic to FP logic instructions. Since we don't have FP logic instructions on f16 type, we don't need set true for it.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5726
   return (VT == MVT::f64 && Subtarget.hasSSE2()) ||
-         (VT == MVT::f32 && Subtarget.hasSSE1()) ||
-         (VT == MVT::f16 && Subtarget.hasFP16());
+         (VT == MVT::f32 && Subtarget.hasSSE1()) || VT == MVT::f16;
 }
----------------
skan wrote:
> Why is this diffferent from `isScalarFPTypeInSSEReg` in X86FastISel.cpp?
> 
> ``` 
> bool isScalarFPTypeInSSEReg(EVT VT) const {
>     return ((VT == MVT::f16 || VT == MVT::f64) && Subtarget->hasSSE2()) ||
>            (VT == MVT::f32 && Subtarget->hasSSE1());
>   }
> ```
Good catch! Unlike `f32` and `f64`, we only use SSE register for `f16`. So no need to check the condition. I'll update the FastISel part, thanks!


================
Comment at: llvm/lib/Target/X86/X86InstrAVX512.td:4101
+                              X86VectorVTInfo _, Predicate prd = HasAVX512> {
+  let Predicates = !if (!eq (prd, HasFP16), [HasFP16], [prd, OptForSize]) in
   def rr : AVX512PI<0x10, MRMSrcReg, (outs _.RC:$dst),
----------------
skan wrote:
> Why do we need compare the `prd` w/ `HasFP16` here?
> Couldn't we just use `[prd, OptForSize]`?
No, we can't. The predicate list AND all its predicates, which means we don't have a pattern for non `OptForSize` case.


================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.td:540
 
+def FR16 : RegisterClass<"X86", [f16], 16, (add FR32)> {let Size = 32;}
+
----------------
skan wrote:
> The alignment is not same as the size?
No. This is spill size instead of alignment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107082/new/

https://reviews.llvm.org/D107082