[llvm-commits] PR6059 patch

[Bob Wilson]

On Jan 25, 2010, at 12:38 PM, Rafael Espindola wrote:

>> 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:
> 
> They are not fully independent, as the proposed fix for correctness
> simplifies the code. I am refactoring the ppc linux case to show that
> it has a similar problem as the ARM one and can be fixed in the same
> way.
> 
> If you are in a hurry, I can finish my ARM patch tonight. My plan was
> to finishing refactoring the linux ppc case first.

No, I'm not in a hurry at all.  Please continue refactoring and cleaning up the code!  It's great stuff.

> 
>> * 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.
> 
> Have I? I noted that "So your patch was already a step in the right direction."

Yes, but you also wrote (in the message that started this thread):
> Attached is a potential fix for PR6059. One way to look at it is that
> it is a partial revert of Bob's previous patch to fix PR5406.


> 
>> * 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.
> 
> The ABI is defined from C/C++ to assembly. The generated IL is an
> implementation detail. If the FE uses i32, the BE can do anything with
> i64. I am fine with adding padding, not adding padding, forcing it to
> memory, asserting or making me coffee :-)

It has been my impression that in cases where it makes sense, LLVM IR should follow the ABI.  In this case, if there is a call that is marked with the AAPCS ABI, and it has i64 arguments, then those i64 values are expected to be passed as specified by AAPCS.

> 
> 
>> * 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.
> 
> I fully agree that this should be in the FE. I started refactoring the
> linux ppc case but I am having a hard time getting qemu-ppc working
> :-(
> 
> Do you know some other target I could test the big endian padding on?

No.  Sorry.