[PowerPC] Handle f16 as a storage type only

[nemanjai]

Summary

The PPC back end currently crashes (fails to select) with f16 input. This patch expands it on subtargets prior to ISA 3.0 (Power9) and uses the HW conversions on Power9.

Fixes https://bugs.llvm.org/show_bug.cgi?id=39865

[shchenz]

lib/Target/PowerPC/PPCISelLowering.cpp

184 ↗

(On Diff #222470)

Do we need to handle ppcf128 also?

lib/Target/PowerPC/PPCInstrVSX.td

114 ↗

(On Diff #222470)

Guard under IsISA3_0?

3263 ↗

(On Diff #222470)

Guard under IsISA3_0?

test/CodeGen/PowerPC/handle-f16-storage-type.ll

8 ↗

(On Diff #222470)

#0 , seems all the function attributes are not defined?

[nemanjai]

lib/Target/PowerPC/PPCISelLowering.cpp

184 ↗

(On Diff #222470)

Not really. That type is really just a pair of doubles and there is no register that can contain it, so it will always be broken up into a pair of doubles by the legalizer.

lib/Target/PowerPC/PPCInstrVSX.td

114 ↗

(On Diff #222470)

Why? This is just a def of a pattern fragment. It is only defined here because it is missing in the target independent td file.

3263 ↗

(On Diff #222470)

The instructions used are defined in a Power9Vector guard and so are these patterns. I realize this is hard to track down from the patch - this file is in desperate need of refactoring :(

test/CodeGen/PowerPC/handle-f16-storage-type.ll

8 ↗

(On Diff #222470)

Sure, I can get rid of these (actually, I'll just define #0 as nounwind and use it for all the functions so we don't get the CFI nodes).

[lei]

lib/Target/PowerPC/PPCInstrVSX.td

3272 ↗

(On Diff #222470)

Should we clear the side effect bit for these? let hasSideEffects = 0

[stefanp]

Other than the test case which still needs to be updated I think that this patch looks good to me.

lib/Target/PowerPC/PPCInstrVSX.td

3272 ↗

(On Diff #222470)

I do not believe that Pat has a Side Effect flag.

The instruction STXSIHX is already marked as let hasSideEffects = 0.
For XSCVDPHP we do want to have side effects because it alters special registers that are not modeled by the operands on the instruction.

[nemanjai]

This should fix https://bugs.llvm.org/show_bug.cgi?id=39865

[nemanjai]

Updated the test case.

[nemanjai]

@lei @stefanp Could you please re-review as this would be good to fix since it fixes a reported bug? I would like to get it committed and backported to V10.0.

[merge_guards_bot]

Unit tests: fail. 62343 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[nemanjai]

My good intentions of updating and uploading the test case were once again thwarted by git in the previous update. Actually included the test case on this one.

[merge_guards_bot]

Unit tests: pass. 62515 tests passed, 0 failed and 844 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[jsji]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

170

Do we need to consider useSoftFloat here?

llvm/lib/Target/PowerPC/PPCInstrVSX.td

114

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?

llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll

35 ↗

(On Diff #242698)

Nit: Function Attrs deleted but comments still kept.

[stefanp]

Mostly nits from me.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

170

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.

llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll

36 ↗

(On Diff #242698)

nit:
The #1 can be removed because it no longer refers to anything.

70 ↗

(On Diff #242698)

nit:
Did you mean to use #0 here instead?

102 ↗

(On Diff #242698)

nit:
The #1 can be removed here too.

[nemanjai]

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

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

170

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.

llvm/lib/Target/PowerPC/PPCInstrVSX.td

114

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.

llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll

35 ↗

(On Diff #242698)

OK.

36 ↗

(On Diff #242698)

OK.

70 ↗

(On Diff #242698)

Yes. Thanks.

[nemanjai]

Fixed up the nits in the test cases and added a test for soft float. Also, moved the pattern fragments to the target independent td file for consistency.

[jsji]

LGTM. Thanks!

[LemonBoy]

Ping? Is this ready to land?

[nemanjai]

In D68237#1975862, @LemonBoy wrote:

Ping? Is this ready to land?

Oh, sorry I kind of forgot about this. Just running the test and I'll commit it if everything still checks out. Thanks for reminding me.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

PPCTargetLowering::PPCTargetLowering(const PPCTargetMachine &TM,
   setOperationAction(ISD::ATOMIC_CMP_SWAP, MVT::i32, Custom);
 
   // PowerPC has an i16 but no i8 (or i1) SEXTLOAD.
   for (MVT VT : MVT::integer_valuetypes()) {
     setLoadExtAction(ISD::SEXTLOAD, VT, MVT::i1, Promote);
     setLoadExtAction(ISD::SEXTLOAD, VT, MVT::i8, Expand);
   }
 
+  if (Subtarget.isISA3_0()) {
+    setLoadExtAction(ISD::EXTLOAD, MVT::f64, MVT::f16, Legal);
+    setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::f16, Legal);
+    setTruncStoreAction(MVT::f64, MVT::f16, Legal);
+    setTruncStoreAction(MVT::f32, MVT::f16, Legal);
+  } else {
+    // No extending loads from f16 or HW conversions back and forth.
+    setLoadExtAction(ISD::EXTLOAD, MVT::f64, MVT::f16, Expand);
+    setOperationAction(ISD::FP16_TO_FP, MVT::f64, Expand);
+    setOperationAction(ISD::FP_TO_FP16, MVT::f64, Expand);
+    setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::f16, Expand);
+    setOperationAction(ISD::FP16_TO_FP, MVT::f32, Expand);
+    setOperationAction(ISD::FP_TO_FP16, MVT::f32, Expand);
+    setTruncStoreAction(MVT::f64, MVT::f16, Expand);
+    setTruncStoreAction(MVT::f32, MVT::f16, Expand);
+  }
+
   setTruncStoreAction(MVT::f64, MVT::f32, Expand);
 
   // PowerPC has pre-inc load and store's.
   setIndexedLoadAction(ISD::PRE_INC, MVT::i1, Legal);
   setIndexedLoadAction(ISD::PRE_INC, MVT::i8, Legal);
   setIndexedLoadAction(ISD::PRE_INC, MVT::i16, Legal);
   setIndexedLoadAction(ISD::PRE_INC, MVT::i32, Legal);
   setIndexedLoadAction(ISD::PRE_INC, MVT::i64, Legal);

llvm/lib/Target/PowerPC/PPCInstrVSX.td

 def PPCswapNoChain : SDNode<"PPCISD::SWAP_NO_CHAIN", SDT_PPCxxswapd>;
 def PPCvabsd : SDNode<"PPCISD::VABSD", SDTVabsd, []>;
 
 def PPCfpexth : SDNode<"PPCISD::FP_EXTEND_HALF", SDT_PPCfpexth, []>;
 def PPCldvsxlh : SDNode<"PPCISD::LD_VSX_LH", SDT_PPCldvsxlh,
                         [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
 def PPCldsplat : SDNode<"PPCISD::LD_SPLAT", SDT_PPCldsplat,
                         [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
+def extloadf16 : PatFrag<(ops node:$ptr), (extload node:$ptr)> {
+  let IsLoad = 1;
+  let MemoryVT = f16;
+}
+def truncstoref16 : PatFrag<(ops node:$val, node:$ptr),
+                            (truncstore node:$val, node:$ptr)> {
+  let IsStore = 1;
+  let MemoryVT = f16;
+}
 
 multiclass XX3Form_Rcr<bits<6> opcode, bits<7> xo, string asmbase,
                     string asmstr, InstrItinClass itin, Intrinsic Int,
                     ValueType OutTy, ValueType InTy> {
   let BaseName = asmbase in {
     def NAME : XX3Form_Rc<opcode, xo, (outs vsrc:$XT), (ins vsrc:$XA, vsrc:$XB),
                        !strconcat(asmbase, !strconcat(" ", asmstr)), itin,
                        [(set OutTy:$XT, (Int InTy:$XA, InTy:$XB))]>;
def : Pat<(v4i32 (scalar_to_vector ScalarLoads.ZELi16)),
             (v4i32 (XXSPLTWs (LXSIHZX xoaddr:$src), 1))>;
   def : Pat<(v2i64 (scalar_to_vector ScalarLoads.ZELi16i64)),
            (v2i64 (XXPERMDIs (LXSIHZX xoaddr:$src), 0))>;
   def : Pat<(v4i32 (scalar_to_vector ScalarLoads.SELi16)),
             (v4i32 (XXSPLTWs (VEXTSH2Ws (LXSIHZX xoaddr:$src)), 1))>;
   def : Pat<(v2i64 (scalar_to_vector ScalarLoads.SELi16i64)),
             (v2i64 (XXPERMDIs (VEXTSH2Ds (LXSIHZX xoaddr:$src)), 0))>;
 
+  // Load/convert and convert/store patterns for f16.
+  def : Pat<(f64 (extloadf16 xoaddr:$src)),
+            (f64 (XSCVHPDP (LXSIHZX xoaddr:$src)))>;
+  def : Pat<(truncstoref16 f64:$src, xoaddr:$dst),
+            (STXSIHX (XSCVDPHP $src), xoaddr:$dst)>;
+  def : Pat<(f32 (extloadf16 xoaddr:$src)),
+            (f32 (COPY_TO_REGCLASS (XSCVHPDP (LXSIHZX xoaddr:$src)), VSSRC))>;
+  def : Pat<(truncstoref16 f32:$src, xoaddr:$dst),
+            (STXSIHX (XSCVDPHP (COPY_TO_REGCLASS $src, VSFRC)), xoaddr:$dst)>;
+
   let Predicates = [IsBigEndian, HasP9Vector] in {
   // Scalar stores of i8
   def : Pat<(truncstorei8 (i32 (vector_extract v16i8:$S, 0)), xoaddr:$dst),
             (STXSIBXv (COPY_TO_REGCLASS (v16i8 (VSLDOI $S, $S, 9)), VSRC), xoaddr:$dst)>;
   def : Pat<(truncstorei8 (i32 (vector_extract v16i8:$S, 1)), xoaddr:$dst),
             (STXSIBXv (COPY_TO_REGCLASS (v16i8 (VSLDOI $S, $S, 10)), VSRC), xoaddr:$dst)>;
   def : Pat<(truncstorei8 (i32 (vector_extract v16i8:$S, 2)), xoaddr:$dst),
             (STXSIBXv (COPY_TO_REGCLASS (v16i8 (VSLDOI $S, $S, 11)), VSRC), xoaddr:$dst)>;