[AArch64] Fix assert AArch64TargetLowering::ReplaceNodeResults

[simonwallis2]

Summary

Don't know how to custom expand this
UNREACHABLE executed at llvm-project/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16788

The fix is to provide missing expansions for:

case ISD::STRICT_FP_TO_UINT:
case ISD::STRICT_FP_TO_SINT:

A test case is provided.

[dmgreen]

Sounds OK to me, from a StrictFP perspective. I just have some test cleanup suggestions/nitpicks.

llvm/test/CodeGen/AArch64/fptosi-strictfp.ll

2

It is commit in llc test to use -mtriple=aarch64-arm-none-eabi as opposed to the datalayout and triple below. But doesn't make a big difference either way.

6

I think you can remove "hidden", and usually "local_unnamed_addr #0"

9

Can we return "conv"? Otherwise something might decide to remove the instruction.

11

Add CHECK-LABEL, and maybe move before the test_fixtfti function? Or just use the update script.

24–25

Can probably remove these.

[simonwallis2]

llvm/test/CodeGen/AArch64/fptosi-strictfp.ll

11

OK I'll add some CHECK-LABELs.
Is there an advantage in moving before the test_fixfti function?
I'm not sure what update script you mean.

[simonwallis2]

Updated test case, actioning most of the review comments.

[dmgreen]

llvm/test/CodeGen/AArch64/fptosi-strictfp.ll

11

It's just more standard - I would expect to find the check lines before the function, where you have added the CHECK-LABEL.

The update script is the llvm/utils/update_llc_test_checks.py script, which can fill in the check lines for you. In this case it won't make much difference, as it is already checking the important parts. For some tests that can change in the future it makes the tests easier to maintain.

I would move the CHECK line next to the CHECK-LABEL to keep them together. Otherwise LGTM.

[simonwallis2]

I have moved some CHECK lines around in the test case, as suggested by reviewer.

[kpn]

llvm/test/CodeGen/AArch64/fptosi-strictfp.ll

24–25

Removing the attributes is incorrect.

All functions that use the strictfp intrinsics require the strictfp attribute on the function definition.

In a function that uses the constrained intrinsics the strictfp attribute is required on all function calls.

This is in the Language Ref. It is not yet in the verifier because last time I tried it we had clang test failures.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

void AArch64TargetLowering::ReplaceNodeResults(
   case AArch64ISD::SMAXV:
     ReplaceReductionResults(N, Results, DAG, ISD::SMAX, AArch64ISD::SMAXV);
     return;
   case AArch64ISD::UMAXV:
     ReplaceReductionResults(N, Results, DAG, ISD::UMAX, AArch64ISD::UMAXV);
     return;
   case ISD::FP_TO_UINT:
   case ISD::FP_TO_SINT:
+  case ISD::STRICT_FP_TO_SINT:
+  case ISD::STRICT_FP_TO_UINT:
     assert(N->getValueType(0) == MVT::i128 && "unexpected illegal conversion");
     // Let normal code take care of it by not adding anything to Results.
     return;
   case ISD::ATOMIC_CMP_SWAP:
     ReplaceCMP_SWAP_128Results(N, Results, DAG, Subtarget);
     return;
   case ISD::LOAD: {
     assert(SDValue(N, 0).getValueType() == MVT::i128 &&

llvm/test/CodeGen/AArch64/fptosi-strictfp.ll

+; RUN: llc < %s | FileCheck %s
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-arm-none-eabi"
+
+define i128 @test_fixtfti(fp128 %ld) {
+; CHECK-LABEL: test_fixtfti:
+; CHECK: bl __fixtfti
+entry:
+  %conv = call i128 @llvm.experimental.constrained.fptosi.i128.f128(fp128 %ld, metadata !"fpexcept.strict")
+  ret i128 %conv
+}
+
+declare i128 @llvm.experimental.constrained.fptosi.i128.f128(fp128, metadata)
+
+define i128 @test_fixtftu(fp128 %ld) {
+; CHECK-LABEL: test_fixtftu:
+; CHECK: bl __fixunstfti
+entry:
+  %conv = call i128 @llvm.experimental.constrained.fptoui.i128.f128(fp128 %ld, metadata !"fpexcept.strict")
+  ret i128 %conv
+}
+
+declare i128 @llvm.experimental.constrained.fptoui.i128.f128(fp128, metadata)