[PATCH] D69578: [AIX] Add support for lowering int, float and double formal arguments.

Wed Nov 13 08:18:16 PST 2019

ZarkoCA added a comment.

In D69578#1741001 <https://reviews.llvm.org/D69578#1741001>, @sfertile wrote:

> A couple high level comments on the tests.
>
> - I like how you extended the 'LowerCall' tests to test both the caller and callee side, as opposed to adding new tests just for the formal argument lowering.
> - There are a lot of tests covering quite a few cases. Its  good to have thorough coverage. Having separate test cases for things like 1 float vs 3 floats, and 1 i8, vs 4 sext i8s, vs 4 zext i8s might be a little //too// verbose though. Having a couple calls that mix various extensions of the i8/i16 types i32 and i64 types should give adequate test coverage. Similarly for the fpr tests, a call which mixes doubles and floats should be adequate. I think the most descriptive test are those that are mixing floating and integer/pointer values. For example calling a function with a signature like: `(i64 %a, double %d, i1 zeroext %b, float %f, i16 signext %c) `  shows how we split the i64 for 32-bit codegen, skip gprs for doubls/floats and map the smaller then register sized types into a gpr all in 1 test.

I removed a few of the somewhat redundant ones, added a few more.

> 
> 
> - Most of the test on the callee side we don't really care about the instructions in the callee, the liveins is all we are really interested in. I find the allocas and dead stores a little odd.  There is no passes to remove them between llc start and where we dump mir so its OK, but I would prefer just having some arbitrary arthritic with the arguments so they are all used to calculate a return value instead, and don't need any checks other then the liveins except in a couple select tests.

I only check for the liveins now tests, and don't check the lines that follow.   I've had some trouble getting tests that have some basic arithmetic to work.  The .ll files are correct but using Filecheck and what it reads and doesn't read has been giving me a lot of trouble.   I can fix them later since I plan to do more work on the callee side but this patch is holding up some other calling convention work and I'd like to get it in before spending more time figuring Filecheck out.

> - We probably want a test like the i1 signext/zext/no flags test I posted in one of the comments, and maybe a similar style test for the codegen we expect for say a type that is smaller then register size but  not an i1.
> - With the reduction in test verbosity (less tests and less CHECK directives) we  should be able to combine `aix_gpr_param.ll` and `aix_fpr_param.ll` into a single file. We will limit that to passing aruments in registers, and will have seperate files for when we spill arguments to memory.
> 
>   I think a few more tests that need us to disable the altivec attr have landed since your last update. If you rebase you will pick them up.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69578/new/

https://reviews.llvm.org/D69578