[llvm-commits] PR6059 patch

[Bob Wilson]

On Jan 22, 2010, at 8:46 PM, Rafael Espindola wrote:

>> Well, that's another thing altogether!  If the purpose of your change is to somehow clean up the argument passing in llvm-gcc, then I'm all in favor.  I don't really know how to do that myself, and I don't think I understand the details of what you're proposing.  I do think it would be a bad idea to add a new hook just for ARM AAPCS.
> 
> Not totally another issue, it is just the first use case that needs a
> more general solution. Another case that can trivially be moved here
> is the special handling we have for AAPCS-VFP.
> 
> Another benefit: Remember the discussion about using byval? With the
> new organization it is easy for the arm backend to find where to split
> the structures that are part in register part in memory. With the
> previous arrangement that would be hard since the code was not in an
> arm specific area.
> 
>> The backend had better be able to correctly handle a function like:
>> 
>> x(i32 arg0, i64 arg1, i64 arg2)
>> 
>> The first argument goes in r0; r1 is unused; arg1 goes in register r2 and r3; and arg2 goes on the stack (with 64-bit alignment).  That is exactly what you're looking for in pr6059, right?
>> 
>> This is the general approach of llvm-gcc: translate the arguments to something that will make the backend do the right thing.  I don't understand why that doesn't work in this case.  Am I missing something?
> 
> The point is that if the FE doesn't produce a i64, "correctly
> handling" the above function can be defined in way that is convenient.
> A silly example of why it is a good idea to lower things early
> 
> struct s {double s; int a}
> void f(int, struct s);
> void g(int a, double b, int c) {
>  struct s x = {s, c};
>  f(a, x);
> }
> 
> If llvm-gcc uses a i64 for the struct and the double, the construction
> of the temporary x (or at least two i64) will survive until code
> generation notices that the i64 will be split. If llvm-gcc is kind
> enough to tell llvm that i32 are going to be used, this get simplified
> very early.

There are several different issues going on here:

* Simplifying llvm-gcc's target hooks for argument passing.
* Correctness issues for ARM.
* Code quality (i.e., the example above about simplifying code earlier)

The changes you've been making so far to clean up argument passing are great.  It sounds like you've got some ideas for improving things further.  I've mostly been focused on correctness, and I think it is generally a good idea to separate out the issues involved in changes like this.  So, ignoring the other issues, here are a few comments on correctness:

* Thinking about pr6059 made me realize that part of my change for pr5406 was completely unnecessary.  In one of your previously proposed patches, you suggested we revert that code, and I agree with you.  There is no need to check if i64 is a legal type when deciding whether to use it to pass aggregates.  The alignment is the only thing that matters.  I've gone ahead and reverted that.  It did not cause any regressions in the GCC compat tests for ARM APCS.

* There is still a problem for pr6059, and it is more general than the original issue.  llvm-gcc now generates correct IR, but the backend is not putting the arguments in the correct registers.  I don't usually work with AAPCS so it's possible that I did something wrong when testing this.

* There is also an open issue for padding structures on big-endian systems.  For PPC, we use "byval" for arguments like this, even when those arguments are actually passed in registers, and the backend figures out where to put the padding.  Big-endian ARM APCS has the same issue, and it is just broken right now.  This really ought to be handled in the frontend, IMO.  While you're thinking about cleaning up the argument passing in llvm-gcc, you might consider something to address this.