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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

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.



- 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.

- 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