[PATCH] D68237: [PowerPC] Handle f16 as a storage type only

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai marked 5 inline comments as done.
nemanjai added a comment.

Thanks for the comments, I'll address them and upload an update shortly.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:170
 
+  if (Subtarget.isISA3_0()) {
+    setLoadExtAction(ISD::EXTLOAD, MVT::f64, MVT::f16, Legal);
----------------
stefanp wrote:
> jsji wrote:
> > Do we need to consider `useSoftFloat` here?
> It's a good question. I did run the test with soft float and it seemed to have worked correctly.
> It appears that type legalization will get rid of all float types ahead of time because of this:
> ```
> if (!useSoftFloat()) {
>     if (hasSPE()) {
>       addRegisterClass(MVT::f32, &PPC::GPRCRegClass);
>       addRegisterClass(MVT::f64, &PPC::SPERCRegClass);
>     } else {
>       addRegisterClass(MVT::f32, &PPC::F4RCRegClass);
>       addRegisterClass(MVT::f64, &PPC::F8RCRegClass);
>     }
>   }
> ```
> So, I think we may be ok. However, I will wait for Nemanja to confirm that what I'm saying makes sense.
Hmm... yeah, I think that's a good question. Presumably with soft float, we shouldn't have a problem since everything is a libcall with GPR parameters, but I'll double check.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:114
                         [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
+def extloadf16 : PatFrag<(ops node:$ptr), (extload node:$ptr)> {
+  let IsLoad = 1;
----------------
jsji wrote:
> Why we need such PatFrag while the other targets with fp16 support not?  Is it due to that we only want to handle storage types in this patch?
> 
> If this is really needed, why not move it to `TargetSelectionDAG.td` to be together with `extloadf32` and `extloadf64`?
I believe that no other targets support extending loads and truncating stores for this type. I can move it to the target independent file for consistency though.


================
Comment at: llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll:35
+
+; Function Attrs: nounwind readnone willreturn
+declare double @llvm.convert.from.fp16.f64(i16) #1
----------------
jsji wrote:
> Nit: Function Attrs deleted but comments still kept.
OK.


================
Comment at: llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll:36
+; Function Attrs: nounwind readnone willreturn
+declare double @llvm.convert.from.fp16.f64(i16) #1
+
----------------
stefanp wrote:
> nit:
> The #1 can be removed because it no longer refers to anything.
OK.


================
Comment at: llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll:70
+; Function Attrs: nofree nounwind writeonly
+define dso_local void @stored(i16* nocapture %a, double %b) local_unnamed_addr #2 {
+; P8-LABEL: stored:
----------------
stefanp wrote:
> nit:
> Did you mean to use #0 here instead?
Yes. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68237