[RISCV][GISel] Do libcall for G_FPTOSI, G_FPTOUI when no D or F support #94613

spaits · 2024-06-06T13:08:25Z

When compiling the following code:

#include <stdio.h>
#include <stdlib.h>
#include <stddef.h>
#include <stdbool.h>

int main() {
    int a;
    float f;
    scanf("%d", &a);

    scanf("%f", &f);
    a += (int)f;
    
    return a;
}

for -march=rv32ima_zbb we get a libcall:

call    scanf
        lw      a0, -20(s0)
        call    __fixsfsi
        mv      a1, a0

When we try to use GlobalISel we get this error:

 error in backend: unable to legalize instruction: %9:_(s32) = G_FPTOSI %8:_(s32) (in function: main)

(Here is a link to a reproducer in Godblot: https://godbolt.org/z/f67vEEb41 )

The goal of this PR is to do a libcall for the legalization of G_FPTOSI and G_FPTOUI instead of doing a fallback to Selection DAG to do the same libcall later.

…ns are not supported

llvmbot · 2024-06-06T14:04:26Z

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-risc-v

Author: Gábor Spaits (spaits)

Changes

When compiling the following code:

#include &lt;stdio.h&gt;
#include &lt;stdlib.h&gt;
#include &lt;stddef.h&gt;
#include &lt;stdbool.h&gt;

int main() {
    int a;
    float f;
    scanf("%d", &amp;a);

    scanf("%f", &amp;f);
    a += (int)f;
    
    return a;
}

for -march=rv32ima_zbb we get a libcall:

call    scanf
        lw      a0, -20(s0)
        call    __fixsfsi
        mv      a1, a0

When we try to use GlobalISel we get this error:

 error in backend: unable to legalize instruction: %9:_(s32) = G_FPTOSI %8:_(s32) (in function: main)

(Here is a link to a reproducer in Godblot: https://godbolt.org/z/f67vEEb41 )

The goal of this PR is to do a libcall for the legalization of G_FPTOSI and G_FPTOUI instead of doing a fallback to Selection DAG to do the same libcall later.

Full diff: https://github.com/llvm/llvm-project/pull/94613.diff

2 Files Affected:

(modified) llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp (+2-1)
(added) llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-fptoi-rv32-libcall.mir (+194)

diff --git a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
index dbfcab7233bf8..69a139957ff23 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
@@ -409,7 +409,8 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST)
   getActionDefinitionsBuilder({G_FPTOSI, G_FPTOUI})
       .legalIf(all(typeInSet(0, {s32, sXLen}), typeIsScalarFPArith(1, ST)))
       .widenScalarToNextPow2(0)
-      .clampScalar(0, s32, sXLen);
+      .clampScalar(0, s32, sXLen)
+      .libcallFor({s32, s32});
 
   getActionDefinitionsBuilder({G_SITOFP, G_UITOFP})
       .legalIf(all(typeIsScalarFPArith(0, ST), typeInSet(1, {s32, sXLen})))
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-fptoi-rv32-libcall.mir b/llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-fptoi-rv32-libcall.mir
new file mode 100644
index 0000000000000..b22338d521a46
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-fptoi-rv32-libcall.mir
@@ -0,0 +1,194 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=riscv32 -mattr=-d -mattr=-f -run-pass=legalizer %s -o - \
+# RUN: | FileCheck %s
+
+---
+name:            fptosi_s1_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptosi_s1_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixsfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s1) = G_FPTOSI %0(s32)
+    %2:_(s32) = G_ANYEXT %1(s1)
+    $x10 = COPY %2(s32)
+    PseudoRET implicit $x10
+
+...
+---
+name:            fptoui_s1_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptoui_s1_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixunssfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s1) = G_FPTOUI %0(s32)
+    %2:_(s32) = G_ANYEXT %1(s1)
+    $x10 = COPY %2(s32)
+    PseudoRET implicit $x10
+
+...
+---
+name:            fptosi_s8_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptosi_s8_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixsfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s8) = G_FPTOSI %0(s32)
+    %2:_(s32) = G_ANYEXT %1(s8)
+    $x10 = COPY %2(s32)
+    PseudoRET implicit $x10
+
+...
+---
+name:            fptoui_s8_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptoui_s8_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixunssfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s8) = G_FPTOUI %0(s32)
+    %2:_(s32) = G_ANYEXT %1(s8)
+    $x10 = COPY %2(s32)
+    PseudoRET implicit $x10
+
+...
+---
+name:            fptosi_s16_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptosi_s16_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixsfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s16) = G_FPTOSI %0(s32)
+    %2:_(s32) = G_ANYEXT %1(s16)
+    $x10 = COPY %2(s32)
+    PseudoRET implicit $x10
+
+...
+---
+name:            fptoui_s16_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptoui_s16_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixunssfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s16) = G_FPTOUI %0(s32)
+    %2:_(s32) = G_ANYEXT %1(s16)
+    $x10 = COPY %2(s32)
+    PseudoRET implicit $x10
+
+...
+---
+name:            fptosi_s32_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptosi_s32_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixsfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s32) = G_FPTOSI %0(s32)
+    $x10 = COPY %1(s32)
+    PseudoRET implicit $x10
+
+...
+---
+name:            fptoui_s32_s32
+body:             |
+  bb.1:
+    liveins: $f10_f
+
+    ; CHECK-LABEL: name: fptoui_s32_s32
+    ; CHECK: liveins: $f10_f
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $f10_f
+    ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: $x10 = COPY [[COPY]](s32)
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-call) &__fixunssfsi, csr_ilp32_lp64, implicit-def $x1, implicit $x10, implicit-def $x10
+    ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $x10
+    ; CHECK-NEXT: $x10 = COPY [[COPY1]](s32)
+    ; CHECK-NEXT: PseudoRET implicit $x10
+    %0:_(s32) = COPY $f10_f
+    %1:_(s32) = G_FPTOUI %0(s32)
+    $x10 = COPY %1(s32)
+    PseudoRET implicit $x10
+
+...

llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-fptoi-rv32-libcall.mir

github-actions · 2024-06-06T21:42:43Z

✅ With the latest revision this PR passed the C/C++ code formatter.

topperc · 2024-06-06T21:48:35Z

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp

@@ -17,6 +17,7 @@
 #include "llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h"
 #include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h"
 #include "llvm/CodeGen/GlobalISel/LegalizerHelper.h"
+#include "llvm/CodeGen/GlobalISel/LegalizerInfo.h"


Why do we need a new include?

topperc · 2024-06-06T21:51:35Z

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp

@@ -169,7 +170,8 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST)
      MergeUnmergeActions.legalIf(
          all(typeIs(BigTyIdx, s64), typeIs(LitTyIdx, s32)));
    }
-    MergeUnmergeActions.widenScalarToNextPow2(LitTyIdx, XLen)
+    MergeUnmergeActions.legalIf(all(typeIs(BigTyIdx, sDoubleXLen), typeIs(LitTyIdx, sXLen)))


How do we isel this UNMERGE?

topperc · 2024-06-06T21:52:33Z

llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-fptoi-rv32.mir

+    ; CHECK-NOFLOAT-LABEL: name: fptosi_s1_s64
+    ; CHECK-NOFLOAT: liveins: $f10_d
+    ; CHECK-NOFLOAT-NEXT: {{  $}}
+    ; CHECK-NOFLOAT-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $f10_d


Something is broken here. We're passing f10_d, but we don't have floating point?

So f10_d is a register only available when D and F are enabled. So I should be using for example x10 as a livein register right?

This also mean that I will have to use a different file for these tests.

For rv32, you would need to use x10 and x11 to hold all 64 bits. I recommend writing an IR test and using -stop-before=legalizer to get the MIR

spaits · 2024-06-06T21:53:02Z

@topperc I have updated the PR. I have discovered, that I could move the test for the libcall cases into the same file in which the non-libcall test are with the help of file check. When running the tests fail in some cases with this:

LLVM ERROR: unable to legalize instruction: %4:_(s32), %5:_(s32) = G_UNMERGE_VALUES %0:_(s64) (in function: fptosi_s1_s64)

So it says that we have an illegal unmerge that can not be legalized. This does not makes sense to me. I was editing the test for merge a few hours back today and I think this should be legalized by the current rules.

I have then realized, that when we are not on an rv32 system that has D extension we never have a legalIf or legalFor call. When I add a legalFor call then the legalization will be fine.

This is the commit, that introduced that if that made the legalFor call for only a specific case: dbb9043 .
Looking at the instruction selection and register bank info I don't really see why we shouldn't have a legalFor when the D extension is not present.

How should I proceed with this PR? Why is there no legal G_UNMERGE call when we have a CPU that is not rv32 and has no D extension?

I would really appreciate your help.

spaits · 2024-06-07T09:29:39Z

@topperc I have removed the legalFor call for G_UNMERGE_VALUES. Also I have added the tests as you have suggested. Now they are using the correct liveins. Now there is no problem with the legality of G_UNMERGE_VALUES for libcalls. Thank you for your help and review.

Could you please re-review this PR?

topperc

LGTM

…rt (llvm#94613) When compiling the following code: ```cpp #include <stdio.h> #include <stdlib.h> #include <stddef.h> #include <stdbool.h> int main() { int a; float f; scanf("%d", &a); scanf("%f", &f); a += (int)f; return a; } ``` for `-march=rv32ima_zbb` we get a libcall: ``` call scanf lw a0, -20(s0) call __fixsfsi mv a1, a0 ``` When we try to use GlobalISel we get this error: ``` error in backend: unable to legalize instruction: %9:_(s32) = G_FPTOSI %8:_(s32) (in function: main) ``` (Here is a link to a reproducer in Godblot: https://godbolt.org/z/f67vEEb41 ) The goal of this PR is to do a libcall for the legalization of `G_FPTOSI` and `G_FPTOUI` instead of doing a fallback to Selection DAG to do the same libcall later. Signed-off-by: Hafidz Muzakky <[email protected]>

[RISCV][GISel] Do libcall for G_FPTOSI, G_FPTOUI when d or f extensio…

22620a9

…ns are not supported

spaits requested review from topperc, michaelmaitland and arsenm June 6, 2024 14:03

spaits marked this pull request as ready for review June 6, 2024 14:03

llvmbot added backend:RISC-V llvm:globalisel labels Jun 6, 2024

topperc reviewed Jun 6, 2024

View reviewed changes

llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-fptoi-rv32-libcall.mir Show resolved Hide resolved

Add tests for rv64

4621d4c

topperc reviewed Jun 6, 2024

View reviewed changes

spaits added 2 commits June 7, 2024 10:49

Add tests for rv32

4c6950f

Modify test for rv32

669868b

spaits force-pushed the riscv-libcall-for-G_FPTOSI branch from d389266 to 669868b Compare June 7, 2024 09:26

spaits requested a review from topperc June 7, 2024 09:29

topperc approved these changes Jun 7, 2024

View reviewed changes

spaits merged commit 28dd55b into llvm:main Jun 7, 2024
7 checks passed

HerrCai0907 mentioned this pull request Jun 13, 2024

tidy #95384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISCV][GISel] Do libcall for G_FPTOSI, G_FPTOUI when no D or F support #94613

[RISCV][GISel] Do libcall for G_FPTOSI, G_FPTOUI when no D or F support #94613

spaits commented Jun 6, 2024 •

edited

Loading

llvmbot commented Jun 6, 2024 •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading

topperc Jun 6, 2024

topperc Jun 6, 2024

topperc Jun 6, 2024

spaits Jun 6, 2024 •

edited

Loading

topperc Jun 6, 2024

spaits commented Jun 6, 2024

spaits commented Jun 7, 2024

topperc left a comment

[RISCV][GISel] Do libcall for G_FPTOSI, G_FPTOUI when no D or F support #94613

[RISCV][GISel] Do libcall for G_FPTOSI, G_FPTOUI when no D or F support #94613

Conversation

spaits commented Jun 6, 2024 • edited Loading

llvmbot commented Jun 6, 2024 • edited Loading

github-actions bot commented Jun 6, 2024 • edited Loading

topperc Jun 6, 2024

Choose a reason for hiding this comment

topperc Jun 6, 2024

Choose a reason for hiding this comment

topperc Jun 6, 2024

Choose a reason for hiding this comment

spaits Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

topperc Jun 6, 2024

Choose a reason for hiding this comment

spaits commented Jun 6, 2024

spaits commented Jun 7, 2024

topperc left a comment

Choose a reason for hiding this comment

spaits commented Jun 6, 2024 •

edited

Loading

llvmbot commented Jun 6, 2024 •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading

spaits Jun 6, 2024 •

edited

Loading