[PATCH] D64662: [FPEnv] [PowerPC] Lower ppc_fp128 StrictFP Nodes to libcalls

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

https://reviews.llvm.org/rL370228 just landed, so I think you should include fptrunc and fpext intrinsics as well.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1913
+  case ISD::FP_ROUND:         Res = ExpandFloatOp_FP_ROUND(N); break;
+  case ISD::STRICT_FP_ROUND:  Res = ExpandFloatOp_STRICT_FP_ROUND(N); break;
+  case ISD::FP_TO_SINT:       Res = ExpandFloatOp_FP_TO_SINT(N); break;
----------------
ajwock wrote:
> ajwock wrote:
> > jsji wrote:
> > > Any reason that we are expanding `STRICT_FP_ROUND ` in `ExpandFloatOperand`, while all the others, especially `STRICT_FP_EXTEND` in `ExpandFloatResult`?
> > I put STRICT_FP_ROUND here for a rather simple-minded reason- FP_ROUND (non strict) is dealt with here as well.  
> > 
> > I couldn't find any documentation or commits on the reason that ExpandFloatOperand_FP_ROUND was here, but this particular addition was required to get clang with the https://reviews.llvm.org/D43142 forceconstrainedfp pass to successfully compile and pass the GNU Scientific Library's tests that involved the long long data type on a PowerPC server.
> Edit: by long long I mean long-double.
I think this is because `STRICT_FP_ROUND` is used for `fptrunc` to `f64`,`f32`, the result type (`f64`,`f32` is already legal here. However, we need to *chain* in the expansion of operands, instead of using default non-strict expansion here. -- I think that is why this expansion is required to get the tests passed.








================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1911
+  case ISD::FP_TO_SINT:       Res = ExpandFloatOp_FP_TO_SINT(N); break;
+  case ISD::FP_TO_UINT:       Res = ExpandFloatOp_FP_TO_UINT(N); break;
+  case ISD::LROUND:           Res = ExpandFloatOp_LROUND(N); break;
----------------
Should also `ExpandFloatOp_STRICT` here for `STRICT_FP_TO_SINT`/`STRICT_FP_TO_UINT`.


================
Comment at: test/CodeGen/PowerPC/ppcf128-constrained-fp-instrinsics.ll:3
+; RUN: llc -O3 -mtriple=powerpc64le-linux-gnu < %s | FileCheck --check-prefix=PC64LE %s
+; RUN: llc -O3 -mtriple=powerpc64le-linux-gnu -mcpu=pwr9 < %s | FileCheck --check-prefix=PC64LE9 %s
+; RUN: llc -O3 -mtriple=powerpc64-linux-gnu < %s | FileCheck --check-prefix=PC64 %s
----------------
Most of  the P8 LE and P9 LE code are actually common, I believe you can simplify the file using check-prefixes.

```
; RUN: llc -O3 -mtriple=powerpc64le-linux-gnu < %s | FileCheck --check-prefixes=LECOMM,PC64LE %s
; RUN: llc -O3 -mtriple=powerpc64le-linux-gnu -mcpu=pwr9 < %s | FileCheck --check-prefixes=LECOMM,PC64LE9 %s
```

then rerun the script should reduce the file.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64662