There was a problem when passing structures as variable arguments. The structures that are smaller than 64 bit were not left justified on MIPS64 big endian. This is fixed by shifting the value to make it left justified when appropriate. This fixes bug http://llvm.org/bugs/show_bug.cgi?id=21608
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Well spotted. I thought I had caught all of these bugs. Sadly it's just missed LLVM 3.6.0.
Could you move the tests into test/CodeGen/Mips/cconv/ and name them arguments-varargs-*.ll? I'm trying to keep all calling convention tests together. With the tests moved and the nits fixed, it will LGTM.
Do you need someone to commit it?
Ideally, they would be of roughly the same form as test/CodeGen/Mips/cconv/arguments-varargs.ll and they'd check the assembly for big/little-endian O32, N32, and N64 like that test does. This bit can be done as a follow-up if need be.
| lib/Target/Mips/MipsCallingConv.td | ||
|---|---|---|
| 162 | I'm not sure the i64 is necessary but it's not doing any harm either so no change required. | |
| test/CodeGen/Mips/small-structs-byte.ll | ||
| 1 | Nit: Are these tests generated from C source? If so, could you add the source file as a comment? The other calling convention tests are (mostly) handwritten LLVM-IR but that doesn't really work for varargs so I've been making an exception for them. | |
| 15 | Nit: This comment isn't useful. Likewise for the other examples in the tests. | |
| test/CodeGen/Mips/small-structs-combinations.ll | ||
| 14 | Nit: Duplicate blank line | |
| test/CodeGen/Mips/small-structs-multiple-args.ll | ||
| 79 | Nit: Blank line | |
Comment Actions
Bugzilla was playing up yesterday but I've been able to look at PR21608 again and there's something I should mention here. I believe this patch delivers the correct and intended behaviour but it should be noted that GCC miscompiles this case in the same way. I mention this because our criteria for all the other calling convention bugs was "matches GCC's behaviour" since it implements the de-facto calling convention for Mips which appears to differ slightly from the documented calling convention. On this occasion, it makes sense for us to differ from GCC's current (broken) behaviour and have GCC fixed later.
Comment Actions
I think that the example provided in PR21608 is not correct.
The reason why I think so is because integers are passed as variable arguments and structures are expected.
I would say that function test_i32(...) should be called like this:
printf("%d\n", test_i32(
"",
(struct f){-1},
(struct f){1},
(struct f){2},
(struct f){3},
(struct f){4},
(struct f){5},
(struct f){6},
(struct f){7},
(struct f){8},
(struct f){9},
(struct f){10}
)
);Now structures are passed instead of integers.
And if you compile this example with GCC it works good.
| lib/Target/Mips/MipsCallingConv.td | ||
|---|---|---|
| 162 | The i64 is used here for structures bigger than 32 bits. | |
Comment Actions
Now that you've mentioned it I see it too. I agree the test is broken. There should be a gcc bugzilla ticket with the same test case somewhere. I'll see if I can track it down and mark it invalid.
| lib/Target/Mips/MipsCallingConv.td | ||
|---|---|---|
| 162 | Good point. Sorry for the noise. | |
Comment Actions
Test files moved to test/CodeGen/Mips/cconv
And renamed to arguments-varargs-*
Also stylistic errors corrected.
I'm not sure the i64 is necessary but it's not doing any harm either so no change required.