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

[Kan Shengchen via Phabricator via cfe-commits]

skan added inline comments.


================
Comment at: llvm/docs/ReleaseNotes.rst:136
 
-* ...
+* Support ``half`` type on SSE2 and above targets.
 
----------------
Just for curiosity, why is SSE2?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:593
 
+    // Half type will be promoted by default.
+    setOperationAction(ISD::FABS, MVT::f16, Promote);
----------------
Promote to which type?


================
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());
 }
----------------
Add comments for `hasBWI`?


================
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;
 }
----------------
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());
  }
```


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20809
 
+  if (VT == MVT::f16 && !Subtarget.hasFP16())
+    return SDValue();
----------------
Need comments


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:21277
 
-  if (DstVT == MVT::f128)
+  if (DstVT == MVT::f128 || (DstVT == MVT::f16 && !Subtarget.hasFP16()))
     return SDValue();
----------------
Need comments


================
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),
----------------
Why do we need compare the `prd` w/ `HasFP16` here?
Couldn't we just use `[prd, OptForSize]`?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107082