On MIPS unsigned int type should not be zero extended, it's sign extended now.
Details
Diff Detail
Event Timeline
This seems to be a backend bug rather than a frontend bug. The backend should be able to handle functions like declare void @foo(i32 signext, i128 signext) correctly but it currently doesn't. The backend has everything it needs (including the alignment) to handle it correctly though. The alignment can be found in ArgFlags.getOrigAlign() and can be checked with CCIfAlign.
It should be possible to fix this in a frontend-independent way by adding something like 'CCIfType<[i64], CCIfAlign<"8", CCAssignToRegWithShadow<[A0_64, A2_64, ...], [D12_64, D14_64, ...]>>>' but it also needs something to mark the skipped registers as being allocated so that subsequent arguments don't try to fill in the gaps this leaves behind.
Another possibility is adding a new action to tablegen (CCRegAlign?) which walks through the unallocated argument registers and marks them allocated until it finds a register with a suitable alignment.
Would you be willing to look into a backend fix for this?
My understanding is that int128 shouldn't be sign extended because it is larger than register size, so I think that is frontend problem.
Second problem, frontend gives zeroext for unsigned int32, it should be either signext or direct (no extension), so I think that is frontend problem, not backend. This can be fixed in another place (CGCall.cpp file, ConstructAttributeList function) but this is common code, so I think current patch is better solution for this.
I tracked down these issues to patch: http://reviews.llvm.org/D5961 .
My priority is fixing unsigned int32 problem.
My understanding is that int128 shouldn't be sign extended because it is larger than register size, so I think that is frontend problem.
That's not quite right. The signext/zeroext attributes specify that caller will fully extend the value to the size of the containers (which may span multiple physical registers). When the container is bigger than the type, it allows the backend to omit sign/zero-extension instructions. When the container is the same size as the type, it makes no difference and isn't harmful. For example, i65 should have the signext attribute even though it's bigger than a GPR in order to fill the 128-bit container (two 64-bit GPR's).
The frontend is just saying that the caller has to deal with the architectural requirements related to sign/zero-extension. It's the backend that needs to know what these requirements are and deal with them. Without the attributes, the backend will arrange for the callee to deal with them instead.
Second problem, frontend gives zeroext for unsigned int32, it should be either signext or direct (no extension), so I
think that is frontend problem, not backend. This can be fixed in another place (CGCall.cpp file, ConstructAttributeList
function) but this is common code, so I think current patch is better solution for this.
This shouldn't be a problem but it is an uncomfortable discrepancy with our _documented_ ABI which says that 32-bit integers are always sign-extended to GPR-sized integers. I put the underbars around 'documented' because the available documentation isn't completely reliable as a statement of how things are actually implemented. I discussed it with Matthew Fortune (one of the current GCC maintainers for Mips) at the time and as far as we could tell, the truth of the matter is that unsigned integers are passed zero extended by the caller in all cases. I also ran a few hundred thousand randomized ABI tests and couldn't produce any failures running mixtures of clang-compiled and gcc-compiled code.
I tracked down these issues to patch: http://reviews.llvm.org/D5961 .
Are you trying to solve a particular bug? If so, it would be useful to see a failing executable testcase.
IIRC from a recent discussion, unsigned 32-bit int are represented in a 64-bit register as sign-extended value of the 32-bit value (i.e. bit 31 is copied through the top half). So, the caller has to make sure the value is correctly represented before making a call, right?
Yes, I'm trying to resolve bug in dejagnu test suite, this is the shorter version of failed test :
extern void exit (int); extern void abort (void); unsigned __attribute__ ((__noinline__)) foo(unsigned a) { unsigned l; l = (a >= (~0u - 512) ? (~0u - 512) : a); return l; } int main (void) { if (foo ((unsigned) -512) != (unsigned) -513) abort (); exit (0); }
That's correct in general, all the 32-bit operations sign-extend their result to 64-bit. The ABI documentation is trying to match this architectural behaviour but that didn't seem to be what was actually implemented in gcc.
Ok, I can see two main differences between gcc and clang in this test. The first is that clang did 'sltiu $1, $4, -513' whereas gcc did 'sltu $3, $4, -512'. When using -mips3, the 'foo' function is otherwise equivalent (just different spellings of the same operations and different delay slot filling) so there's probably something wrong there.
The second may be the one that pointed you in this direction: clang is using 'daddiu $4, $0, -512' while gcc is using 'li $4, -512. These should produce the same register contents though (0xffff ffff ffff ffff fe00) so the off-by-one error above sounds more likely to be the bug causing the test to fail. That said, I'm thinking that there is a bug in the application of zeroext attributes as you say. i16 definitely needs zeroext. i31 and i33 should use zeroext too but it seems that i32 should signext. So maybe the rule should be 'unsigned integers always use zeroext, except i32 which uses signext'.
My understanding is that the called function should assume sign-extended (both signed and unsigned) 32-bit integers as inputs on its entry and compiler can base decisions on that. One [1] of Strahinja's previous patches was led by a bug when floating point value was NOT sign-extended but zero-extended before the call, and that led to an incorrect behaviour in the called function. For that particular case, GCC was sign-extending the value before making a call.
I know this area is not well defined, this is why I brought another example to take into account before any decision is made.
There are two solutions for i32 signext problem. First, i32 argument pass directly (in that case i32 will be sign extended) this fix is in MIPS part.
Second solution is fix in common code as I mentioned (CGCall.cpp file, ConstructAttributeList function). This would required new hook in TargetInfo. I think first solution is better for this problem.
Unfortunately, it's even less well defined in the documentation for softfloat float's. I believe both compilers treat them as if they were 32-bit integers which seems like the sensible thing to do.
I agree that the first solution is the better approach but it shouldn't be passed direct. That would cause the backend to sign-extend on the callee side as well as the caller side. This happens because directly passed arguments have undefined upper bits and the operations sometimes (e.g. comparisons, shifts, etc.) require a sign/zero extension. It should be passed signext.
Thanks. I think this patch is heading in the right direction. I have a lingering doubt about how ABIArgInfo::Extend is used in a number of places but we're only changing one of them. I looked through the others and didn't see anything that needed changing though. @atanasyan, do you mind taking a look too?
One other thing I'm thinking about is that we ought to keep the code to handle this in the same place as the rest (in MipsABIInfo::classifyArgumentType()). This would require adding an ABIArgInfo::SignExtend.
Also, have you tried a variant of your failing test but using varargs? This uses a different code path so we ought to check that too.
lib/CodeGen/TargetInfo.cpp | ||
---|---|---|
5793–5797 | Nit: Please add a comment explaining why unsigned i32 should be sign-extended. It won't be obvious to people who don't already know about this quirk of the ABI. |
Also, have you tried a variant of your failing test but using varargs? This uses a different code path so we ought to check that too.
@spetrovic: I haven't seen a reply to this. Did I miss it?
lib/CodeGen/TargetInfo.cpp | ||
---|---|---|
5793–5797 | @spetrovic: I haven't seen a reply to this. |
lib/CodeGen/ABIInfo.h | ||
---|---|---|
93 ↗ | (On Diff #24686) | Nit: Just for consistency let's:
|
lib/CodeGen/TargetInfo.cpp | ||
5796 | Nit: else-after-return violation. return Ty->isUnsignedIntegerOrEnumerationType() && TySize == 32; or if (Ty->isUnsignedIntegerOrEnumerationType() && TySize == 32) return true; return false; And +1 for comment |
All comments addressed. One more test added with varargs, this patch also fixes vararg case (without patch unsigned int is ZEROEXT instead SIGNEXT).
LGTM with a spelling nit in the new comment
lib/CodeGen/TargetInfo.cpp | ||
---|---|---|
5795 | Nit: Spelling of 'integers' |
Nit: Spelling of 'integers'