llvm dev - Oct 2012 - [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.

On Mon, Oct 22, 2012 at 11:23 AM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>> 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.
>
>> To simplify review, I've attached two patches, one with the actual
fix (and
>> a new test for it), and one that contains all the signext/zeroext test
>> changes.
>>
>> Would this be OK to commit?
>>
>> Bye,
>> Ulrich
>
>
> Cheers,
> Rafael
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

On 22 October 2012 14:53, Chandler Carruth <chandlerc at google.com>
wrote:
> 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.

Cheers,
Rafael

On 22/10/12 21:12, Rafael Espíndola wrote:
> On 22 October 2012 14:53, Chandler Carruth <chandlerc at google.com>
wrote:
>> 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.

Ciao, Duncan.

Chandler Carruth <chandlerc at google.com> wrote on 22.10.2012 20:53:54:
> 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 :-)

> 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. ]
> On Mon, Oct 22, 2012 at 11:23 AM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
> >> 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