[cfe-commits] [PATCH/RFC, PowerPC] Extend 32-bit function arguments / return values

So, I'm not really sure if this is the right approach. I'd like some
folks from the LLVM side of things to chime in.

In general, I'm not certain we want to continue growing our dependence
on the signext and zeroext attributes on return types, or whether we
want to do the extension in the frontend instead.

Most of the targets in Clang currently eagerly zext or sext the return
value to make it conform to the ABI. You can look at some of the other
classify*Type methods in Clang for how.

Unfortunately, this change also causes about 20 spurious regression test
failures on PowerPC, since IR output now frequently (i.e. for plain "int"
types) contains an extra "signext" attribute, which throws off strict
text-matching in various tests. I've fixed those by optionally accepting
"signext" (or "zeroext" as the case may be) via a regexp. Not sure if
this is really the best way of handling this ...

Not sure as well. It looks easy to forget to add this to new tests and
break a ppc64 bot. What abound adding some -triple= to the tests?
LGTM with that change if you are ok with it. Just give it a day to see
if anyone else has another idea.

I'd vote for not adding optional matching to the tests. Instead, we
should specify an exact triple and match the expected IR exactly.
However, let's figure out what the IR should look like first.

As far as I know the difference is enabling optimization. If we see a

declare i8 zeroext @foo()

the caller knows that the top bits of the return are 0. There was some
discussion about just using the range metadata, but that is not
available for arguments/returns at the moment.

Cheers,
Rafael

Chris wrote some notes about this:
   http://www.nondot.org/sabre/LLVMNotes/ExtendedIntegerResults.txt
The plan seems sensible to me, but was blocked by not having a good
way of attaching the information to parameters and return values.
Hopefully Bill's attribute work means it is now possible.

Ciao, Duncan.

So, I'm not really sure if this is the right approach. I'd like some
folks from the LLVM side of things to chime in.

In general, I'm not certain we want to continue growing our dependence
on the signext and zeroext attributes on return types, or whether we
want to do the extension in the frontend instead.

I don't really have any strong opinion one way or the other whether it
is preferable to do the extension in the front end or the back end; I'd
be happy to implement whatever you feel best. However, clang currently
generates code that is definitely incompatible with ABI requirements on
PPC64 (only in some, but rather frequent, special cases), so I was hoping
to find a way to fix this specific codegen bug without requiring too much
of a redesign of the whole thing :slight_smile:

Most of the targets in Clang currently eagerly zext or sext the return
value to make it conform to the ABI. You can look at some of the other
classify*Type methods in Clang for how.

Would you mind elaborating where to look, specifically? What I'm seeing
is that some classify*Type methods select different types, like e.g.:

    return ABIArgInfo::getDirect(llvm::Type::getInt64Ty(getVMContext()));

I was hoping this could be used to implement extension for ABI purposes,
but it doesn't look like this will work (with the current infrastructure).

If the type specified in the classify*Type method is larger than the
actual parameter/return value type, CGCall.cpp will in fact create
extend operations, but those will always be zero-extends:
/// CoerceIntOrPtrToIntOrPtr - Convert a value Val to the specific Ty where
both
/// are either integers or pointers. This does a truncation of the value
if it
/// is too large or a zero extension if it is too small.

Since the PPC64 ABI specifies sign- or zero-extension (depending on
the signedness of the original argument type at the source level),
this doesn't seem to help.

Did I miss some other method that would allow for sign-extends?

However, in any case, even if the extension were implemented fully in
the front-end, I'd *still* run into the very same test case problem:
"int" return/argument types would then be represented as "i64" in the
IR, which still wouldn't match test cases that are hard-coded to
expect "i32". In fact, no matter what, those tests would fail, since
just plain "i32" with no attributes cannot express the semantics
required by the ABI, and any change from plain "i32" will cause the
test cases to break ... So it seems to me the question of how to
deal with the tests is independent of the question whether to extend
in the front end or the back end.

[ B.t.w. doesn't LLVM support (or plan to support) targets where
"int" isn't a 32-bit type in the first place? On such targets all
those tests would already break as well, in any case. ]

>> Unfortunately, this change also causes about 20 spurious regression

test

>> failures on PowerPC, since IR output now frequently (i.e. for plain

"int"

>> types) contains an extra "signext" attribute, which throws off strict
>> text-matching in various tests. I've fixed those by optionally

accepting

>> "signext" (or "zeroext" as the case may be) via a regexp. Not sure

if

>> this is really the best way of handling this ...
>
> Not sure as well. It looks easy to forget to add this to new tests and
> break a ppc64 bot. What abound adding some -triple= to the tests?
> LGTM with that change if you are ok with it. Just give it a day to see
> if anyone else has another idea.

I'd vote for not adding optional matching to the tests. Instead, we
should specify an exact triple and match the expected IR exactly.
However, let's figure out what the IR should look like first.

Well, I guess the "exact triple" would specify Intel, right? In which
case the IR wouldn't really have to change ...

However, this would mean a whole bunch of test cases either wouldn't
be executed at all on non-Intel platforms, or else we'd have to have
(near-)duplicates of the tests for different platforms. Neither option
looks really desirable to me.

If we want to check for exact matches, what about the following approach:
Have the test case dynamically determine which IR type an "int" source-
level type corresponds to on the target (e.g. by compiling a dummy
routine and capturing the output in a FileCheck variable), and then check
for that same result in the other tests in the file.

Thanks,
Ulrich

Well, I guess the "exact triple" would specify Intel, right? In which
case the IR wouldn't really have to change ...

However, this would mean a whole bunch of test cases either wouldn't
be executed at all on non-Intel platforms, or else we'd have to have
(near-)duplicates of the tests for different platforms. Neither option
looks really desirable to me.

There are not that many tests and they are not very platform
dependent, so I don't thing you are losing too much by adding a triple
to them.

Thanks,
Ulrich

Cheers,
Rafael

Well, fine with me. I can prepare a patch to add the triple.

Which triple would you suggest to use for this?

i386-apple-darwin ?
i386-pc-linux-gnu ?
i386-unknown-unknown ?

(I don't have Darwin systems to test on myself. I could test either of
the latter on Linux/Intel ...)

Thanks,
Ulrich

Which triple would you suggest to use for this?

i386-apple-darwin ?
i386-pc-linux-gnu ?
i386-unknown-unknown ?

(I don't have Darwin systems to test on myself. I could test either of
the latter on Linux/Intel ...)

If they pass with i386-unknown-unknown, that is probably the best.

Thanks,
Ulrich

Thanks,
Rafael

From: "Duncan Sands" <baldrick@free.fr>
To: "Rafael Espíndola" <rafael.espindola@gmail.com>
Cc: "Ulrich Weigand" <Ulrich.Weigand@de.ibm.com>, "llvm cfe" <cfe-commits@cs.uiuc.edu>, "LLVM Developers Mailing
List" <llvmdev@cs.uiuc.edu>
Sent: Tuesday, October 23, 2012 2:13:37 AM
Subject: Re: [LLVMdev] [cfe-commits] [PATCH/RFC, PowerPC] Extend 32-bit function arguments / return values

>> So, I'm not really sure if this is the right approach. I'd like
>> some
>> folks from the LLVM side of things to chime in.
>>
>> In general, I'm not certain we want to continue growing our
>> dependence
>> on the signext and zeroext attributes on return types, or whether
>> we
>> want to do the extension in the frontend instead.
>>
>> Most of the targets in Clang currently eagerly zext or sext the
>> return
>> value to make it conform to the ABI. You can look at some of the
>> other
>> classify*Type methods in Clang for how.
>
> As far as I know the difference is enabling optimization. If we see
> a
>
> declare i8 zeroext @foo()
>
> the caller knows that the top bits of the return are 0. There was
> some
> discussion about just using the range metadata, but that is not
> available for arguments/returns at the moment.

Chris wrote some notes about this:
   http://www.nondot.org/sabre/LLVMNotes/ExtendedIntegerResults.txt
The plan seems sensible to me, but was blocked by not having a good
way of attaching the information to parameters and return values.
Hopefully Bill's attribute work means it is now possible.

Indeed. Are you proposing that Ulrich hold off on the current fix in favor of the new attribute scheme, or that he should fix this using the current mechanism for now (perhaps with a FIXME to upgrade to the new scheme once it is in place)? I would prefer for him to commit this now, and then upgrade later.

Thanks again,
Hal

OK, the appended patch implements this. All tests pass (both on a
PowerPC-Linux and a Intel-Linux host), and continue passing with my
original ppc64 ABI patch added, as expected.

Bye,
Ulrich

(See attached file: diff-clang-ppc64-extend-testfixes)

diff-clang-ppc64-extend-testfixes (11.2 KB)

OK, the appended patch implements this. All tests pass (both on a
PowerPC-Linux and a Intel-Linux host), and continue passing with my
original ppc64 ABI patch added, as expected.

LGTM. We will need something like that with any of the proposed solutions.

Bye,
Ulrich

Cheers,
Rafael

Hi Hal,

Chris wrote some notes about this:
    http://www.nondot.org/sabre/LLVMNotes/ExtendedIntegerResults.txt
The plan seems sensible to me, but was blocked by not having a good
way of attaching the information to parameters and return values.
Hopefully Bill's attribute work means it is now possible.

Indeed. Are you proposing that Ulrich hold off on the current fix in favor of the new attribute scheme, or that he should fix this using the current mechanism for now (perhaps with a FIXME to upgrade to the new scheme once it is in place)? I would prefer for him to commit this now, and then upgrade later.

I don't have anything useful to say about Ulrich's fix since I didn't look at
it. I just wanted to make sure that everyone knew where we are probably going:
in the direction laid out by Chris in those notes.

Ciao, Duncan.

solutions.

Checked in as r166551.

Thanks,
Ulrich

>> Chris wrote some notes about this:
>> http://www.nondot.org/sabre/LLVMNotes/ExtendedIntegerResults.txt
>> The plan seems sensible to me, but was blocked by not having a good
>> way of attaching the information to parameters and return values.
>> Hopefully Bill's attribute work means it is now possible.
>
> Indeed. Are you proposing that Ulrich hold off on the current fix
in favor of the new attribute scheme, or that he should fix this
using the current mechanism for now (perhaps with a FIXME to upgrade
to the new scheme once it is in place)? I would prefer for him to
commit this now, and then upgrade later.

I don't have anything useful to say about Ulrich's fix since I didn't

look at

it. I just wanted to make sure that everyone knew where we are
probably going:
in the direction laid out by Chris in those notes.

I'm wondering how to proceed on this issue for now. Current status is
that I've checked in the testsuite patch, since there was agreement
that fixing the tests by adding target triples was the way to go.
However, the actual fix to enable the sign-extensions required by
the PowerPC64 ABI is not in, so we still generate code that violates
the ABI.

I understand that there is a long-term goal of having those extensions
done in the front end instead of in LLVM. However, as far as I can
see the necessary infrastructure is not yet fully present (or in any
event, I don't see how to do it in the front end right now) ...

Therefore, since my patch doesn't make moving to the new scheme
any more difficult (it just does the same thing for "int" that is
already being done for "short" and "char"), and it does fix the
ABI bug we have right now, I'd propose to check it in now (and
then move to new scheme with everyone else once it is ready).

Would this be OK? Any other suggestions?

For reference, the patch in question is appended again.

Thanks,
Ulrich

(See attached file: diff-clang-ppc64-extend)

diff-clang-ppc64-extend (3.34 KB)

Please factor out the check for whether an integer type needs to be
extended into a separate method.

Please make sure there's a testcase on the LLVM side to make sure we
actually handle the attributes correctly.

Otherwise, looks fine.

-Eli