Skip to content

[Mips] Fix clang crashes when compiling a variadic function while targeting mips3 #130558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 16, 2025
Merged

[Mips] Fix clang crashes when compiling a variadic function while targeting mips3 #130558

merged 1 commit into from
Apr 16, 2025

Conversation

yingopq
Copy link
Contributor

@yingopq yingopq commented Mar 10, 2025 •
edited by MaskRay
Loading

issue reason:
Because mips3 has the feature 'FeatureGP64Bit', when target mips3 process function writeVarArgRegs, the result of getGPRSizeInBytes is 8 and the result of GetVarArgRegs is Mips::A0, Mips::A1, Mips::A2, Mips::A3. This would generate gpr64 = COPY $a1 which should be gpr64 = COPY $a1_64.

Fix #98716.

@yingopq yingopq force-pushed the Fix_bug_issue_98716 branch from 38ea768 to 1a7f4e4 Compare March 10, 2025 07:41
@yingopq yingopq force-pushed the Fix_bug_issue_98716 branch from 1a7f4e4 to 0960960 Compare March 19, 2025 02:40
@brad0 brad0 requested review from wzssyqa and topperc March 19, 2025 08:00
@brad0
Copy link
Contributor

brad0 commented Mar 23, 2025

cc @wzssyqa @topperc

@yingopq
Copy link
Contributor Author

yingopq commented Mar 31, 2025

@wzssyqa Could you help review this patch, the author of the issue ticket has verified this issue for us, and the verification result is OK, thanks.

@brad0
Copy link
Contributor

brad0 commented Apr 6, 2025

@wzssyqa @topperc Ping.

@brad0
Copy link
Contributor

brad0 commented Apr 12, 2025

cc @topperc @MaskRay

MaskRay
MaskRay approved these changes Apr 12, 2025
View reviewed changes
llvm/test/CodeGen/Mips/variable-function-arguments.ll Outdated
@@ -0,0 +1,53 @@
; RUN: llc -mtriple=mipsel-linux-gnu -mcpu=mips3 -target-abi o32 < %s | FileCheck %s -check-prefixes=MIPS3-O32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps call this vararg.ll. Quite a few targets have a test named this.

Re-generate with update_llc_test_checks.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, applied.

llvm/lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp Outdated
ArrayRef<MCPhysReg> MipsABIInfo::GetVarArgRegs() const {
if (IsO32())
return ArrayRef(O32IntRegs);
ArrayRef<MCPhysReg> MipsABIInfo::GetVarArgRegs(bool isGP64bit) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to getVarArgRegs (functionName convention) while you are modifying it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, applied.

@MaskRay MaskRay changed the title [Mips] Fix clang crashes when compiling a variadic function while tar… [Mips] Fix clang crashes when compiling a variadic function while targeting mips3 Apr 12, 2025
@MaskRay
Copy link
Member

MaskRay commented Apr 12, 2025

The title was truncated due to github. It's more readable to ignore github and fix the title.

Because mips3 has the feature 'FeatureGP64

It seems to apply to newer processors as well.

@yingopq yingopq force-pushed the Fix_bug_issue_98716 branch from 0960960 to 995248d Compare April 14, 2025 07:47
@yingopq
Copy link
Contributor Author

yingopq commented Apr 14, 2025

The title was truncated due to github. It's more readable to ignore github and fix the title.

Thanks!

@yingopq
Copy link
Contributor Author

yingopq commented Apr 14, 2025

Because mips3 has the feature 'FeatureGP64

It seems to apply to newer processors as well.

Now, I only see mips3 use this feature.

def FeatureMips3       : SubtargetFeature<"mips3", "MipsArchVersion", "Mips3",
                                "MIPS III ISA Support [highly experimental]",
                                [FeatureMips2, FeatureMips3_32,
                                 FeatureMips3_32r2, FeatureGP64Bit,
                                 FeatureFP64Bit]>;

@yingopq
Copy link
Contributor Author

yingopq commented Apr 14, 2025

@MaskRay Could you help review again, thanks!

@yingopq
[Mips] Fix clang crashes when compiling a variadic function while tar…
01c732e 
…geting mips3

issue reason:
Because mips3 has the feature 'FeatureGP64Bit', when target mips3 process
function `writeVarArgRegs`, the result of `getGPRSizeInBytes` is 8 and
the result of `GetVarArgRegs` is `Mips::A0, Mips::A1, Mips::A2, Mips::A3`.
This would generate `gpr64 = COPY $a1` which should be `gpr64 = COPY $a1_64`.

Also when process `CC_Mips_FixedArg`, mips would CCDelegateTo
`CC_MipsO32_FP`. In fact, it should CCDelegateTo `CC_MipsN`.

Fix llvm#98716.
@yingopq yingopq force-pushed the Fix_bug_issue_98716 branch from 995248d to 01c732e Compare April 15, 2025 10:00
@MaskRay
Copy link
Member

MaskRay commented Apr 15, 2025

Because mips3 has the feature 'FeatureGP64

It seems to apply to newer processors as well.

Now, I only see mips3 use this feature.

def FeatureMips3       : SubtargetFeature<"mips3", "MipsArchVersion", "Mips3",
                                "MIPS III ISA Support [highly experimental]",
                                [FeatureMips2, FeatureMips3_32,
                                 FeatureMips3_32r2, FeatureGP64Bit,
                                 FeatureFP64Bit]>;

FeatureMips4 implies FeatureMips3 and FeatureMips5 implies FeatureMips4, so newer processors get this feature as well.

@yingopq
Copy link
Contributor Author

yingopq commented Apr 16, 2025

FeatureMips4 implies FeatureMips3 and FeatureMips5 implies FeatureMips4, so newer processors get this feature as well.

OK, thanks!
Did you know why occur this failing check LLVM Premerge Checks / LInux Premerge Checks? Can we ignore this failing results and commit it directly?

@brad0
Copy link
Contributor

brad0 commented Apr 16, 2025

Did you know why occur this failing check LLVM Premerge Checks / LInux Premerge Checks? Can we ignore this failing results and commit it directly?

It says to ignore it.

@yingopq
Copy link
Contributor Author

yingopq commented Apr 16, 2025

Did you know why occur this failing check LLVM Premerge Checks / LInux Premerge Checks? Can we ignore this failing results and commit it directly?

It says to ignore it.

OK, thanks!

@yingopq yingopq merged commit e676866 into llvm:main Apr 16, 2025
10 of 11 checks passed
var-const pushed a commit to ldionne/llvm-project that referenced this pull request Apr 17, 2025
@yingopq @var-const
[Mips] Fix clang crashes when compiling a variadic function while tar…
952d5bf 
…geting mips3 (llvm#130558)

issue reason:
Because mips3 has the feature 'FeatureGP64Bit', when target mips3
process function `writeVarArgRegs`, the result of `getGPRSizeInBytes` is
8 and the result of `GetVarArgRegs` is `Mips::A0, Mips::A1, Mips::A2,
Mips::A3`. This would generate `gpr64 = COPY $a1` which should be `gpr64
= COPY $a1_64`.

Also when process `CC_Mips_FixedArg`, mips would CCDelegateTo
    `CC_MipsO32_FP`. In fact, it should CCDelegateTo `CC_MipsN`.

Fix llvm#98716.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaskRay MaskRay MaskRay approved these changes

@wzssyqa wzssyqa Awaiting requested review from wzssyqa

@topperc topperc Awaiting requested review from topperc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clang crashes when compiling a variadic function while targeting mips3
3 participants
@yingopq @brad0 @MaskRay