This is an archive of the discontinued LLVM Phabricator instance.

[clang][PowerPC] Add flag to enable compatibility with GNU for complex arguments
Needs ReviewPublic

Authored by long5hot on Mar 27 2023, 3:32 AM.

Details

Reviewers
nemanjai
jhibbits
sfertile
Group Reviewers
Restricted Project
Summary

PR :https://github.com/llvm/llvm-project/issues/56023

https://godbolt.org/z/1bsW1sKMs

newFlag: -fcomplex-ppc-gnu-abi

GNU uses GPRs for complex parameters and return values storing for PowerPC-32bit, which can be enabled which above flag.
Intent of this patch is to make clang compatible with GNU libraries of complex.\

added newTestcase below

Diff Detail

Event Timeline

umesh.kalappa0 created this revision.Mar 27 2023, 3:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 3:32 AM
umesh.kalappa0 requested review of this revision.Mar 27 2023, 3:32 AM
umesh.kalappa0 edited the summary of this revision. (Show Details)Mar 27 2023, 3:38 AM
umesh.kalappa0 edited the summary of this revision. (Show Details)Mar 28 2023, 12:47 AM
umesh.kalappa0 removed a reviewer: nikic.
brad added a subscriber: kernigh.Mar 28 2023, 1:28 AM
brad added a subscriber: brad.
nikic edited reviewers, added: Restricted Project; removed: nikic.Mar 28 2023, 11:49 PM
long5hot commandeered this revision.Jul 17 2023, 8:26 AM
long5hot added a reviewer: umesh.kalappa0.
long5hot edited reviewers, added: jhibbits, sfertile; removed: umesh.kalappa0.Jul 17 2023, 8:29 AM
long5hot edited subscribers, added: sfertile, umesh.kalappa0; removed: shchenz, StephenFan, cfe-commits.
long5hot updated this revision to Diff 541065.EditedJul 17 2023, 8:48 AM

-fcomplex-ppc-gnu-abi switch .

long5hot edited the summary of this revision. (Show Details)Jul 17 2023, 9:07 AM
long5hot edited the summary of this revision. (Show Details)
MaskRay added inline comments.Jul 17 2023, 9:26 AM
clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c
3

Use %clang_cc1 for non-driver tests.

For driver tests, replaced long-deprecated -target with --target=

long5hot updated this revision to Diff 541111.Jul 17 2023, 10:03 AM

updating testCase

long5hot updated this revision to Diff 541321.Jul 17 2023, 10:24 PM

changing sequence(order) of arguments in testcase because of failure.

daltenty added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5419

small drive by nit: this seems like an ELF specific driver option.

long5hot updated this revision to Diff 541849.Jul 18 2023, 11:30 PM

Extra check regarding ELF.

long5hot added inline comments.Jul 18 2023, 11:43 PM
clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c
6

do not the reason for testCase failing..
In my Linux x86_64 Deb distro it passes.
But here the failure is, for some reason it's not taking my flag as input and storing complex on stack.

DavidSpickett added inline comments.
clang/include/clang/Driver/Options.td
2100

"stack", lower case s

clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c
7

Super minor nitpick: you have an extra space before the | here (stood out when I was comparing the lines).

PowerPC ABI incompatibility with GNU on complex arguments

Can you rephrase the title to be more descriptive? How about: "[clang][PowerPC] Add flag to enable compatibility with GNU for complex arguments"

As it is currently it sounds more like a bug report than a fix for a bug report.

long5hot updated this revision to Diff 542473.Jul 20 2023, 6:01 AM
long5hot retitled this revision from PowerPC ABI incompatibility with GNU on complex arguments to [clang][PowerPC] Add flag to enable compatibility with GNU for complex arguments.

addressing @DavidSpickett's comments..

nemanjai added inline comments.Aug 8 2023, 8:06 AM
clang/lib/CodeGen/Targets/PPC.cpp
348

I am not familiar with this code, but it seems strange to me that we use alignment to determine whether to use int32 or int64. Does this break if we're compiling something with __attribute__((aligned(...))) or is the "unadjusted alignment" not affected by this?

Also, perhaps an assert that Ty is complex would be useful here.

This diff doesn't work for me. I applied the diff to commit 250812 (Aug 9). clang accepts the flag -fcomplex-ppc-gnu-abi, and is running clang -cc1 with the same flag; but the flag has no effect on the generated code, and the test fails at the first CHECK-GNU-LABEL line.

MaskRay added inline comments.Aug 9 2023, 10:25 PM
clang/include/clang/Driver/Options.td
2100

No trailing dot.

clang/lib/Driver/ToolChains/Clang.cpp
5423

Search render in this file.

long5hot updated this revision to Diff 548966.Aug 10 2023, 4:22 AM
long5hot edited the summary of this revision. (Show Details)

The revision till now was working for release build only.
Wasn't aware of round-trip in assert-build.
Updating accrodingly.

Thanks, @long5hot. Your diff fixes compatibility with gcc for some functions. These 2 functions are broken,

float complex
scale1(int n, float complex c)
{
	return n * c;
}

double complex
scale2(int n, double complex c)
{
	return n * c;
}

The compilers differ at r4,

  • gcc does scale1(int r3, float complex r5:r6) and scale2(int r3, double complex r4:r5:r6:r7). See https://godbolt.org/z/xjdWaWP3q
  • clang -fcomplex-ppc-gnu-abi does scale1(int r3, float complex r4:r5) and scale2(int r3, double complex r5:r6:r7:r8).

So gcc is weird; the alignment of complex arguments in gprs doesn't match their alignment in memory. A 2x32-bit float complex has 64-bit alignment (skip r4) but a 2x64-bit double complex has 32-bit alignment (don't skip r4). This is an accident, as gcc confuses complex args with integer args: a float complex has the same 64-bit size as a long long, so gcc applies long long's rule (odd:even gpr pair); a double complex has a different size and misses the odd:even rule.

About the name, -fcomplex-ppc-gnu-abi begins with -f but I guess that most PowerPC-specific options begin with -m.

kernigh added inline comments.Aug 11 2023, 3:26 PM
clang/lib/CodeGen/Targets/PPC.cpp
354–360

This edit fixes my functions scale1(int, float complex) and scale2(int, double complex). I am compiling these functions with clang -fcomplex-ppc-gnu-abi targetting powerpc-unknown-openbsd, and their caller with gcc 8.4.0.

nemanjai added inline comments.Aug 13 2023, 12:54 AM
clang/lib/CodeGen/Targets/PPC.cpp
354–360

Just out of curiosity, would the compiler do the right thing if we were to simplify this to just use an integer type half the size of the complex type (i.e. the size of one of the elements)?

Something like:

return ABIArgInfo::getDirect(llvm::ArrayType::get(
    llvm::Type::getIntNTy(getVMContext(), TypeSize / 2), 2));

This will use one of [2 x i32], [2 x i64], [2 x i128] and presumably the legalizer/call lowering will do the right thing.

Thanks, @long5hot. Your diff fixes compatibility with gcc for some functions. These 2 functions are broken,

float complex
scale1(int n, float complex c)
{
	return n * c;
}

double complex
scale2(int n, double complex c)
{
	return n * c;
}

The compilers differ at r4,

  • gcc does scale1(int r3, float complex r5:r6) and scale2(int r3, double complex r4:r5:r6:r7). See https://godbolt.org/z/xjdWaWP3q
  • clang -fcomplex-ppc-gnu-abi does scale1(int r3, float complex r4:r5) and scale2(int r3, double complex r5:r6:r7:r8).

So gcc is weird; the alignment of complex arguments in gprs doesn't match their alignment in memory. A 2x32-bit float complex has 64-bit alignment (skip r4) but a 2x64-bit double complex has 32-bit alignment (don't skip r4). This is an accident, as gcc confuses complex args with integer args: a float complex has the same 64-bit size as a long long, so gcc applies long long's rule (odd:even gpr pair); a double complex has a different size and misses the odd:even rule.

The thing is GCC is following ABI, which states,

according to ATR-PASS-COMPLEX-IN-GPRS

complex single-precision float : If gr is even, set gr = gr + 1. Load the lower-addressed word of the
argument into gr and the higher-addressed word into gr + 1, set gr = gr + 2.

complex double-precision float: Load the words of the argument, in memory-address order, into gr, gr + 1,
gr + 2 and gr + 3, set gr = gr + 4.

About the name, -fcomplex-ppc-gnu-abi begins with -f but I guess that most PowerPC-specific options begin with -m.

The flag was given -f because actual usecase is till frontend. if we use -m then it will come in target-features.

long5hot updated this revision to Diff 549866.Aug 14 2023, 3:42 AM

updating according to ATR-PASS-COMPLEX-IN-GPRS.
Thanks @kernigh!

clang/lib/CodeGen/Targets/PPC.cpp
354–360

Thanks!

354–360

Idea was same that, lower to [2 x i32], [ 2 x i64].. , But to follow ABI for complex float and doubles i think we need to use @kernigh's patch

kernigh added inline comments.Aug 29 2023, 2:27 PM
clang/include/clang/Driver/Options.td
2098
.../Options.td:2424:98: error: Element type mismatch for list: element type 'OptionVisibility' not convertible to 'OptionFlag

I broke it again. This function doesn't work,

#include "complex.h"

double complex
third(int no, double complex no_again, double complex yes)
{
    return yes;
}

https://godbolt.org/z/jcWKxn68x has gcc passing yes on the stack. clang -Xclang -fcomplex-ppc-gnu-abi now passes the first 12 bytes of yes in registers r8, r9, r10, and the last 4 bytes on the stack. I guess that PPC32_SVR4_ABIInfo::handleComplex needs to check how many registers are left?