[Hexagon] Bugfix in VarArg lowering: Special case for musl #157564

benedikt-l-huber · 2025-09-08T21:57:02Z

While debugging #145206 I found that a possible cause for the problem is
the call to printf, which is variadic.

In a musl environment VarArgs are treated like non VarArgs.
The handling of this special case was bypassed by the commit
a4f8551.

The reason is that the arguement TreatAsVarArg is only set to true in
an non musl env. TreatAsVarArg determines the result of isVarArg().
The CCIfArgVarArg class only checks each individual
variable, but not whether isVarArg() is true.

Without the special case, the unnamed arguments are always passed
on the stack, as is standard calling convention. But with musl
also unnamed arguments can be passed in registers.

Possibly, this fixes #145206.

When compiling for musl varargs are treated like non varargs. Thus the check must also include the State.isVarArg(), since this is determined by the TreatAsVarArg argument, that depends on the musl environment. Possibly, this fixes llvm#145206.

github-actions · 2025-09-08T21:57:24Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-09-08T21:57:56Z

@llvm/pr-subscribers-backend-hexagon

Author: Benedikt Lukas Huber (benedikt-l-huber)

Changes

While debugging #145206 I found that a possible cause for the problem is
the call to printf, which is variadic.

In a musl environment VarArgs are treated like non VarArgs.
The handling of this special case was bypassed by the commit
a4f8551.

The reason is that the arguement TreatAsVarArg is only set to true in
an non musl env. TreatAsVarArg determines the result of isVarArg().
The CCIfArgVarArg class only checks each individual
variable, but not whether isVarArg() is true.

Without the special case, the unnamed arguments are always passed
on the stack, as is standard calling convention. But with musl
also unnamed arguments can be passed in registers.

Possibly, this fixes #145206.

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

2 Files Affected:

(modified) llvm/lib/Target/Hexagon/HexagonCallingConv.td (+6-2)
(added) llvm/test/CodeGen/Hexagon/vararg-musl.ll (+15)

diff --git a/llvm/lib/Target/Hexagon/HexagonCallingConv.td b/llvm/lib/Target/Hexagon/HexagonCallingConv.td
index fd6d873dd4188..96c41cdb2b7ee 100644
--- a/llvm/lib/Target/Hexagon/HexagonCallingConv.td
+++ b/llvm/lib/Target/Hexagon/HexagonCallingConv.td
@@ -6,6 +6,10 @@
 //
 //===----------------------------------------------------------------------===//
 
+
+class CCIfBothVarArgAndArgVarArg<CCAction A>
+  : CCIf<"State.isVarArg() && ArgFlags.isVarArg()", A>;
+
 def CC_HexagonStack: CallingConv<[
   CCIfType<[i32,v2i16,v4i8],
     CCAssignToStack<4,4>>,
@@ -23,7 +27,7 @@ def CC_Hexagon_Legacy: CallingConv<[
 
   CCIfByVal<
     CCPassByVal<8,8>>,
-  CCIfArgVarArg<
+  CCIfBothVarArgAndArgVarArg<
     CCDelegateTo<CC_HexagonStack>>,
 
   // Pass split values in pairs, allocate odd register if necessary.
@@ -53,7 +57,7 @@ def CC_Hexagon: CallingConv<[
 
   CCIfByVal<
     CCPassByVal<8,1>>,
-  CCIfArgVarArg<
+  CCIfBothVarArgAndArgVarArg<
     CCDelegateTo<CC_HexagonStack>>,
 
   // Pass split values in pairs, allocate odd register if necessary.
diff --git a/llvm/test/CodeGen/Hexagon/vararg-musl.ll b/llvm/test/CodeGen/Hexagon/vararg-musl.ll
new file mode 100644
index 0000000000000..b902dded32153
--- /dev/null
+++ b/llvm/test/CodeGen/Hexagon/vararg-musl.ll
@@ -0,0 +1,15 @@
+; RUN: llc -mtriple=hexagon-unknown-linux-musl < %s | FileCheck %s -check-prefix=MUSL
+; RUN: llc -mtriple=hexagon-unknown-none-elf   < %s | FileCheck %s -check-prefix=NONMUSL
+
+; MUSL-NOT: memw
+; NONMUSL: memw
+
+declare i32 @f0(i32 %a0, ...)
+
+define i32 @f1(i32 %a0, i32 %a1) #0 {
+b1:
+  %v7 = call i32 (i32, ...) @f0(i32 %a0, i32 %a1)
+  ret i32 %v7
+}
+
+attributes #0 = { nounwind }

benedikt-l-huber · 2025-09-08T22:02:21Z

@aankit-ca could you review this?

androm3da · 2025-09-08T22:07:07Z

If this is a musl-specific bug then perhaps the solution must also be musl-specific? Should this change be bound to the specific triple?

benedikt-l-huber · 2025-09-08T22:14:59Z

A possibility would be to reintroduce HexagonCCState and let it hold the information about the musl environment, taken from Subtarget.isEnvironmentMusl().

androm3da · 2025-09-10T12:04:50Z

HexagonCCState

I'm not familiar enough to say whether this is the right solution - maybe @aankit-ca or @iajbar can comment?

androm3da · 2025-09-10T12:06:31Z

llvm/test/CodeGen/Hexagon/vararg-musl.ll

+; RUN: llc -mtriple=hexagon-unknown-linux-musl < %s | FileCheck %s -check-prefix=MUSL
+; RUN: llc -mtriple=hexagon-unknown-none-elf   < %s | FileCheck %s -check-prefix=NONMUSL


Instead of (or in addition to?) this test, maybe we should add RUN lines to some of the existing vararg tests?

$ grep RUN llvm/test/CodeGen/Hexagon/vararg* llvm/test/CodeGen/Hexagon/vararg_align_check.ll:; RUN: llc -mtriple=hexagon -mcpu=hexagonv62 -mtriple=hexagon-unknown-linux-musl -O0 < %s | FileCheck %s llvm/test/CodeGen/Hexagon/vararg-deallocate-sp.ll:; RUN: llc -mtriple=hexagon -mtriple=hexagon-unknown-linux-musl < %s | FileCheck %s llvm/test/CodeGen/Hexagon/vararg_double_onstack.ll:; RUN: llc -mtriple=hexagon -mcpu=hexagonv62 -mtriple=hexagon-unknown-linux-musl -O0 < %s | FileCheck %s llvm/test/CodeGen/Hexagon/vararg-formal.ll:; RUN: llc -mtriple=hexagon < %s | FileCheck %s llvm/test/CodeGen/Hexagon/vararg-linux-abi.ll:; RUN: llc -mtriple=hexagon -mtriple=hexagon-unknown-linux-musl < %s | FileCheck %s llvm/test/CodeGen/Hexagon/vararg.ll:; RUN: llc -mtriple=hexagon -mcpu=hexagonv62 -mtriple=hexagon-unknown-linux-musl -O0 < %s | FileCheck %s llvm/test/CodeGen/Hexagon/vararg_named.ll:; RUN: llc -mtriple=hexagon -mcpu=hexagonv62 -mtriple=hexagon-unknown-linux-musl -O0 < %s | FileCheck %s llvm/test/CodeGen/Hexagon/varargs-memv.ll:; RUN: llc -mtriple=hexagon < %s

I have looked into the vararg tests and did not immediately find
one that could be readily adapted. The best candidate would be
varargs-memv.ll. However this one also checks for something else.
On the other hand, there may be some value in having a simple
dedicated test like vararg-musl.ll.
If anybody can point me to a suitable test, I can make the adaption,
otherwise I would suggest to just keep vararg-musl.ll as it is.

aankit-ca

This looks like a reasonable change to me. I don't think there's any advantage of reintroducing the HexagonCCState

llvm/lib/Target/Hexagon/HexagonCallingConv.td

androm3da · 2025-09-15T22:12:02Z

This looks like a reasonable change to me. I don't think there's any advantage of reintroducing the HexagonCCState

If this vararg change is working well on the baseline for baremetal/qurt/hexagon-unknown-elf then changing it generally for the sake of linux-musl feels like it could cause some breakage. @aankit-ca if you're willing to cherry-pick this change and run some more comprehensive verification, then perhaps it would make sense to take the change as-is.

aankit-ca · 2025-09-16T18:58:03Z

@androm3da This change should not affect non musl environments and I've verified it. The CCState is initialized with TreatAsVarArg which is defined like this

  // Linux ABI treats var-arg calls the same way as regular ones.
  bool TreatAsVarArg = !Subtarget.isEnvironmentMusl() && IsVarArg;

At the moment, this TreatAsVarArg is ignored. The State.isVarArg() should check for this value after this change.

aankit-ca · 2025-09-16T19:32:33Z

@benedikt-l-huber Can you just address the minor comments that are pending on the PR?

benedikt-l-huber · 2025-09-17T12:49:38Z

@aankit-ca Done.
How should we proceed with the vararg tests?
When the PR is ready for merge, I will squash and rebase it first.

github-actions · 2025-09-17T18:03:49Z

@benedikt-l-huber Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

aankit-ca · 2025-09-17T18:04:12Z

@aankit-ca Done. How should we proceed with the vararg tests? When the PR is ready for merge, I will squash and rebase it first.

I think the test you added was fine. Thank you for the patch!

aankit-ca · 2025-09-17T18:07:08Z

/cherry-pick e56b479

llvmbot · 2025-09-17T18:13:11Z

/cherry-pick e56b479

Error: Command failed due to missing milestone.

aankit-ca · 2025-09-17T19:42:09Z

/cherry-pick e56b479

I guess this change is not needed on 21.0 since the breaking change was merged only on main

llvmbot · 2025-09-17T19:44:18Z

Failed to cherry-pick: e56b479

https://github.com/llvm/llvm-project/actions/runs/17808727033

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

llvmbot · 2025-09-17T19:48:23Z

Failed to cherry-pick: e56b479

https://github.com/llvm/llvm-project/actions/runs/17808821563

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

androm3da · 2025-09-25T04:22:52Z

/cherry-pick e56b479

I guess this change is not needed on 21.0 since the breaking change was merged only on main

That doesn't sound right to me, @aankit-ca. #145206 was reported against 20.1.5. So the problem has probably been there "forever" -- or at least "a while". Can you fix whatever conflicts show up when cherry-picking back to 21.x?

llvmbot · 2025-09-25T04:28:48Z

Failed to cherry-pick: e56b479

https://github.com/llvm/llvm-project/actions/runs/17996834674

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

aankit-ca · 2025-09-29T17:16:44Z

I see, thanks Brian. I assumed the cause of the failure was this commit a4f8551 which was merged just on main. @benedikt-l-huber Can you please cherry-pick these changes for 21.x?

androm3da · 2025-10-14T04:18:42Z

/cherry-pick e56b479

llvmbot · 2025-10-14T04:25:04Z

Failed to cherry-pick: e56b479

https://github.com/llvm/llvm-project/actions/runs/18485335007

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

llvmbot added the backend:Hexagon label Sep 8, 2025

androm3da requested review from SidManning and aankit-ca September 8, 2025 22:02

androm3da reviewed Sep 10, 2025

View reviewed changes

aankit-ca reviewed Sep 15, 2025

View reviewed changes

llvm/lib/Target/Hexagon/HexagonCallingConv.td Outdated Show resolved Hide resolved

llvm/lib/Target/Hexagon/HexagonCallingConv.td Outdated Show resolved Hide resolved

[Hexagon] CallingConv rename class and explain in comment

e376fc7

benedikt-l-huber requested review from aankit-ca and androm3da September 17, 2025 12:49

androm3da approved these changes Sep 17, 2025

View reviewed changes

aankit-ca merged commit e56b479 into llvm:main Sep 17, 2025
9 checks passed

aankit-ca added this to the LLVM 21.x Release milestone Sep 17, 2025

github-project-automation bot added this to LLVM Release Status Sep 17, 2025

github-project-automation bot moved this to Needs Triage in LLVM Release Status Sep 17, 2025

llvmbot added the release:cherry-pick-failed label Sep 17, 2025

tstellar moved this from Needs Triage to Won't Merge in LLVM Release Status Sep 23, 2025

		; RUN: llc -mtriple=hexagon-unknown-linux-musl < %s \| FileCheck %s -check-prefix=MUSL
		; RUN: llc -mtriple=hexagon-unknown-none-elf < %s \| FileCheck %s -check-prefix=NONMUSL

[Hexagon] Bugfix in VarArg lowering: Special case for musl #157564

[Hexagon] Bugfix in VarArg lowering: Special case for musl #157564

Uh oh!

Conversation

benedikt-l-huber commented Sep 8, 2025

Uh oh!

github-actions bot commented Sep 8, 2025

Uh oh!

llvmbot commented Sep 8, 2025

Uh oh!

benedikt-l-huber commented Sep 8, 2025

Uh oh!

androm3da commented Sep 8, 2025

Uh oh!

benedikt-l-huber commented Sep 8, 2025

Uh oh!

androm3da commented Sep 10, 2025

Uh oh!

androm3da Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

benedikt-l-huber Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

aankit-ca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

androm3da commented Sep 15, 2025

Uh oh!

aankit-ca commented Sep 16, 2025

Uh oh!

aankit-ca commented Sep 16, 2025

Uh oh!

benedikt-l-huber commented Sep 17, 2025

Uh oh!

Uh oh!

github-actions bot commented Sep 17, 2025

Uh oh!

aankit-ca commented Sep 17, 2025

Uh oh!

aankit-ca commented Sep 17, 2025

Uh oh!

llvmbot commented Sep 17, 2025

Uh oh!

aankit-ca commented Sep 17, 2025

Uh oh!

llvmbot commented Sep 17, 2025

Uh oh!

llvmbot commented Sep 17, 2025

Uh oh!

androm3da commented Sep 25, 2025

Uh oh!

llvmbot commented Sep 25, 2025

Uh oh!

aankit-ca commented Sep 29, 2025

Uh oh!

androm3da commented Oct 14, 2025

Uh oh!

llvmbot commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants