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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Mostly nits from me.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:170
 
+  if (Subtarget.isISA3_0()) {
+    setLoadExtAction(ISD::EXTLOAD, MVT::f64, MVT::f16, Legal);
----------------
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.


================
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
+
----------------
nit:
The #1 can be removed because it no longer refers to anything.


================
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:
----------------
nit:
Did you mean to use #0 here instead?


================
Comment at: llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll:102
+; Function Attrs: nounwind readnone willreturn
+declare i16 @llvm.convert.to.fp16.f64(double) #1
+
----------------
nit:
The #1 can be removed here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68237