[SDag][ARM][RISCV] Allow lowering CTPOP into a libcall #99752

s-barannikov · 2024-07-20T10:15:15Z

The main change is adding CTPOP to RuntimeLibcalls.def to allow
targets to use LibCall action for CTPOP. DAG legalizers are changed
accordingly.

llvmbot · 2024-07-20T10:15:44Z

@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-backend-loongarch
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-selectiondag

Author: Sergei Barannikov (s-barannikov)

Changes

Some in-tree targets (e.g. ARM) could benefit from this lowering, at least in opt-size mode, but I don't know if the libcall is available in their libgcc, so no tests.

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

2 Files Affected:

(modified) llvm/include/llvm/IR/RuntimeLibcalls.def (+3)
(modified) llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp (+21-21)

diff --git a/llvm/include/llvm/IR/RuntimeLibcalls.def b/llvm/include/llvm/IR/RuntimeLibcalls.def
index 89aaf6d1ad83f..3dd75622b8e43 100644
--- a/llvm/include/llvm/IR/RuntimeLibcalls.def
+++ b/llvm/include/llvm/IR/RuntimeLibcalls.def
@@ -85,6 +85,9 @@ HANDLE_LIBCALL(NEG_I64, "__negdi2")
 HANDLE_LIBCALL(CTLZ_I32, "__clzsi2")
 HANDLE_LIBCALL(CTLZ_I64, "__clzdi2")
 HANDLE_LIBCALL(CTLZ_I128, "__clzti2")
+HANDLE_LIBCALL(CTPOP_I32, "__popcountsi2")
+HANDLE_LIBCALL(CTPOP_I64, "__popcountdi2")
+HANDLE_LIBCALL(CTPOP_I128, "__popcountti2")
 
 // Floating-point
 HANDLE_LIBCALL(ADD_F32, "__addsf3")
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
index 9f515739ee048..c0141cfc533b7 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -140,12 +140,9 @@ class SelectionDAGLegalize {
                        RTLIB::Libcall Call_F128,
                        RTLIB::Libcall Call_PPCF128,
                        SmallVectorImpl<SDValue> &Results);
-  SDValue ExpandIntLibCall(SDNode *Node, bool isSigned,
-                           RTLIB::Libcall Call_I8,
-                           RTLIB::Libcall Call_I16,
-                           RTLIB::Libcall Call_I32,
-                           RTLIB::Libcall Call_I64,
-                           RTLIB::Libcall Call_I128);
+  SDValue ExpandIntLibCall(SDNode *Node, bool IsSigned, RTLIB::Libcall Call_I8,
+                           RTLIB::Libcall Call_I16, RTLIB::Libcall Call_I32,
+                           RTLIB::Libcall Call_I64, RTLIB::Libcall Call_I128);
   void ExpandArgFPLibCall(SDNode *Node,
                           RTLIB::Libcall Call_F32, RTLIB::Libcall Call_F64,
                           RTLIB::Libcall Call_F80, RTLIB::Libcall Call_F128,
@@ -2209,7 +2206,7 @@ void SelectionDAGLegalize::ExpandFPLibCall(SDNode* Node,
   ExpandFPLibCall(Node, LC, Results);
 }
 
-SDValue SelectionDAGLegalize::ExpandIntLibCall(SDNode* Node, bool isSigned,
+SDValue SelectionDAGLegalize::ExpandIntLibCall(SDNode *Node, bool IsSigned,
                                                RTLIB::Libcall Call_I8,
                                                RTLIB::Libcall Call_I16,
                                                RTLIB::Libcall Call_I32,
@@ -2224,7 +2221,9 @@ SDValue SelectionDAGLegalize::ExpandIntLibCall(SDNode* Node, bool isSigned,
   case MVT::i64:  LC = Call_I64; break;
   case MVT::i128: LC = Call_I128; break;
   }
-  return ExpandLibCall(LC, Node, isSigned).first;
+  assert(LC != RTLIB::UNKNOWN_LIBCALL &&
+         "LibCall explicitly requested, but not available");
+  return ExpandLibCall(LC, Node, IsSigned).first;
 }
 
 /// Expand the node to a libcall based on first argument type (for instance
@@ -4980,19 +4979,20 @@ void SelectionDAGLegalize::ConvertNodeToLibcall(SDNode *Node) {
                                        RTLIB::MUL_I64, RTLIB::MUL_I128));
     break;
   case ISD::CTLZ_ZERO_UNDEF:
-    switch (Node->getSimpleValueType(0).SimpleTy) {
-    default:
-      llvm_unreachable("LibCall explicitly requested, but not available");
-    case MVT::i32:
-      Results.push_back(ExpandLibCall(RTLIB::CTLZ_I32, Node, false).first);
-      break;
-    case MVT::i64:
-      Results.push_back(ExpandLibCall(RTLIB::CTLZ_I64, Node, false).first);
-      break;
-    case MVT::i128:
-      Results.push_back(ExpandLibCall(RTLIB::CTLZ_I128, Node, false).first);
-      break;
-    }
+    Results.push_back(ExpandIntLibCall(Node, /*IsSigned=*/false,
+                                       RTLIB::UNKNOWN_LIBCALL,
+                                       RTLIB::UNKNOWN_LIBCALL,
+                                       RTLIB::CTLZ_I32,
+                                       RTLIB::CTLZ_I64,
+                                       RTLIB::CTLZ_I128));
+    break;
+  case ISD::CTPOP:
+    Results.push_back(ExpandIntLibCall(Node, /*IsSigned=*/false,
+                                       RTLIB::UNKNOWN_LIBCALL,
+                                       RTLIB::UNKNOWN_LIBCALL,
+                                       RTLIB::CTPOP_I32,
+                                       RTLIB::CTPOP_I64,
+                                       RTLIB::CTPOP_I128));
     break;
   case ISD::RESET_FPENV: {
     // It is legalized to call 'fesetenv(FE_DFL_ENV)'. On most targets

github-actions · 2024-07-20T10:18:59Z

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

Some in-tree targets (e.g. ARM) could benefit from this lowering, at least in opt-size mode, but I don't know if the libcall is available in their libgcc, so no tests.

tschuett · 2024-07-20T18:19:27Z

Dead code?

arsenm · 2024-07-21T08:15:12Z

but I don't know if the libcall is available in their libgcc

Should be easy to find out?

s-barannikov · 2024-07-21T08:51:43Z

but I don't know if the libcall is available in their libgcc

Should be easy to find out?

Right. It appears it is available and gcc uses it for __builtin_popcount[l,ll] even when optimizing for speed (-Ofast).
There is no 128-bit version though.
I'll update the PR.

topperc · 2024-07-21T18:19:34Z

llvm/test/CodeGen/RISCV/rv64-legal-i32/rv64xtheadbb.ll

 ; RV64I-NEXT:    ld ra, 8(sp) # 8-byte Folded Reload
 ; RV64I-NEXT:    addi sp, sp, 16
+; RV64I-NEXT:    sext.w a0, a0


__popcountsi2 should be returning a sign extended result right?

Nevermind, this is probably a bug specific to rv64-legal-i32 which is still experimental.

I tried setting IsSExt to true in MakeLibCallOptions, but that changed nothing.

It appears 32-bit libcall is not available in 64-bit libgcc. I reverted to Expand action for rv64-legal-i32.

s-barannikov · 2024-07-29T16:19:33Z

ping

jhuber6 · 2024-07-29T16:20:41Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

+  SDValue ExpandIntLibCall(SDNode *Node, bool IsSigned, RTLIB::Libcall Call_I8,
+                           RTLIB::Libcall Call_I16, RTLIB::Libcall Call_I32,
+                           RTLIB::Libcall Call_I64, RTLIB::Libcall Call_I128);


Looks like a lot of unrelated format changes?

I renamed isSigned -> IsSigned while I was here, clang-format insisted that I reformat this code fragment.
Do I need to revert these changes?

Ah, didn't notice the capital letter change. I don't think it's necessary to change then.

Can precommit this unrelated change

s-barannikov · 2024-08-01T19:47:32Z

@topperc
Please take a look. Are RISC-V changes OK?

topperc · 2024-08-01T20:38:41Z

@topperc Please take a look. Are RISC-V changes OK?

RISC-V changes look ok.

s-barannikov · 2024-08-02T13:26:55Z

This is likely causing armv7/v8 stage2 build failures (other changes don't look related):
https://lab.llvm.org/buildbot/#/builders/135/builds/554
https://lab.llvm.org/buildbot/#/builders/122/builds/389
https://lab.llvm.org/buildbot/#/builders/79/builds/552

I don't have an ARM machine handy to investigate, so I'll just revert the ARM changes and see if it helps.

DavidSpickett · 2024-08-02T13:45:53Z

@antmox should be able to help you figure it out, he's maintaining the bots this week.

s-barannikov · 2024-08-02T14:00:04Z

@antmox Could you please take a look? I've already reverted the changes, but I'd like to reland them if they're not the reason of the failures.

s-barannikov · 2024-08-02T19:31:10Z

The reversal made the bots green.

I think I understood what the issue is. All popcount libcalls returns int, but ISD::CTPOP returns the type of the argument, which can be larger. In type legalizer I need to pass the correct return type to makeLibCall and sign-extend the result afterwards.

I'll revert the rest of the patch because the issue should affect RISC-V as well.

This reverts commit 92e18ff.

) Reverts the rest of #99752

The main change is adding CTPOP to `RuntimeLibcalls.def` to allow targets to use LibCall action for CTPOP. DAG legalizers are changed accordingly.

This is a reland of #99752 with the bug fixed (see test diff in the third commit in this PR). All `popcount` libcalls return `int`, but `ISD::CTPOP` returns the type of the argument, which can be wider than `int`. The fix is to make DAG legalizer pass the correct return type to `makeLibCall` and sign-extend the result afterwards. Original commit message: The main change is adding CTPOP to `RuntimeLibcalls.def` to allow targets to use LibCall action for CTPOP. DAG legalizers are changed accordingly. Pull Request: #101786

This is a reland of llvm#99752 with the bug fixed (see test diff in the third commit in this PR). All `popcount` libcalls return `int`, but `ISD::CTPOP` returns the type of the argument, which can be wider than `int`. The fix is to make DAG legalizer pass the correct return type to `makeLibCall` and sign-extend the result afterwards. Original commit message: The main change is adding CTPOP to `RuntimeLibcalls.def` to allow targets to use LibCall action for CTPOP. DAG legalizers are changed accordingly. Pull Request: llvm#101786

s-barannikov requested review from arsenm, topperc, jhuber6 and mgehre-amd July 20, 2024 10:15

llvmbot added llvm:SelectionDAG SelectionDAGISel as well llvm:ir labels Jul 20, 2024

[SelectionDAG] Allow lowering CTPOP into a libcall

bb8a735

Some in-tree targets (e.g. ARM) could benefit from this lowering, at least in opt-size mode, but I don't know if the libcall is available in their libgcc, so no tests.

s-barannikov force-pushed the popcount-libcall branch from e5b5979 to bb8a735 Compare July 20, 2024 10:40

[ARM] Lower 32-bit CTPOP to a libcall

437a0e5

llvmbot added the backend:ARM label Jul 21, 2024

s-barannikov marked this pull request as draft July 21, 2024 10:12

[SelectionDAG] Legalize wide CTPOP via a libcall

f90a6b4

s-barannikov requested a review from RKSimon July 21, 2024 12:28

s-barannikov changed the title ~~[SelectionDAG] Allow lowering CTPOP into a libcall~~ [SDag][ARM][RISCV] Lower 64-bit CTPOP into a libcall Jul 21, 2024

[RISCV] Expand 64-bit CTPOP into a libcall

66ae5db

s-barannikov force-pushed the popcount-libcall branch from 79cb32d to 66ae5db Compare July 21, 2024 12:46

s-barannikov marked this pull request as ready for review July 21, 2024 12:51

llvmbot added backend:RISC-V backend:loongarch llvm:analysis labels Jul 21, 2024

s-barannikov changed the title ~~[SDag][ARM][RISCV] Lower 64-bit CTPOP into a libcall~~ [SDag][ARM][RISCV] Allow lowering CTPOP into a libcall Jul 21, 2024

[RISCV] Use libcall expansion for RV32, too

724bc32

topperc reviewed Jul 21, 2024

View reviewed changes

jhuber6 reviewed Jul 29, 2024

View reviewed changes

Make type legalizer create a libcall only if explicitly requested

c418f1c

s-barannikov requested review from topperc, efriedma-quic and davemgreen July 29, 2024 18:35

[RISCV] 32-bit popcount is not available in 64-bit libgcc

fb985db

arsenm approved these changes Jul 30, 2024

View reviewed changes

s-barannikov added 2 commits July 30, 2024 14:18

Merge branch 'main' into popcount-libcall

ae297d6

Merge branch 'main' into popcount-libcall

6b48d10

s-barannikov merged commit 92e18ff into llvm:main Aug 2, 2024
7 checks passed

s-barannikov deleted the popcount-libcall branch August 2, 2024 19:31

s-barannikov added a commit that referenced this pull request Aug 2, 2024

Revert "[SDag][ARM][RISCV] Allow lowering CTPOP into a libcall (#99752)"

dcf1296

This reverts commit 92e18ff.

s-barannikov mentioned this pull request Aug 2, 2024

Revert "[SDag][ARM][RISCV] Allow lowering CTPOP into a libcall" #101740

Merged

s-barannikov restored the popcount-libcall branch August 2, 2024 19:45

s-barannikov added a commit that referenced this pull request Aug 2, 2024

Revert "[SDag][ARM][RISCV] Allow lowering CTPOP into a libcall" (#101740

4527fba

) Reverts the rest of #99752

s-barannikov deleted the popcount-libcall branch August 3, 2024 03:51

s-barannikov mentioned this pull request Aug 3, 2024

[SDag][ARM][RISCV] Allow lowering CTPOP into a libcall #101786

Merged

[SDag][ARM][RISCV] Allow lowering CTPOP into a libcall #99752

[SDag][ARM][RISCV] Allow lowering CTPOP into a libcall #99752

Uh oh!

Conversation

s-barannikov commented Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tschuett commented Jul 20, 2024

Uh oh!

arsenm commented Jul 21, 2024

Uh oh!

s-barannikov commented Jul 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-barannikov commented Jul 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-barannikov commented Aug 1, 2024

Uh oh!

topperc commented Aug 1, 2024

Uh oh!

Uh oh!

s-barannikov commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidSpickett commented Aug 2, 2024

Uh oh!

s-barannikov commented Aug 2, 2024

Uh oh!

s-barannikov commented Aug 2, 2024

Uh oh!

Uh oh!

s-barannikov commented Jul 20, 2024 •

edited

Loading

llvmbot commented Jul 20, 2024 •

edited

Loading

github-actions bot commented Jul 20, 2024 •

edited

Loading

s-barannikov commented Aug 2, 2024 •

edited

Loading