Page MenuHomePhabricator

[MIPS] fix extension of integer types (function calls)
ClosedPublic

Authored by spetrovic on Apr 22 2015, 9:09 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

spetrovic updated this revision to Diff 24233.Apr 22 2015, 9:09 AM
spetrovic retitled this revision from to [MIPS] fix extension of integer types (function calls).
spetrovic updated this object.
spetrovic edited the test plan for this revision. (Show Details)
spetrovic added reviewers: dsanders, petarj.
spetrovic added a subscriber: Unknown Object (MLST).
dsanders edited edge metadata.Apr 23 2015, 7:11 AM

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.

petarj edited edge metadata.Apr 27 2015, 8:37 AM

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.

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);
}

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.

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?

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.

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);
}

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

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?

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.

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.

[1] http://reviews.llvm.org/D7791

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.

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?

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.

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.

[1] http://reviews.llvm.org/D7791

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.

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.

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.

spetrovic updated this revision to Diff 24686.Apr 30 2015, 2:10 AM
spetrovic updated this object.
spetrovic edited edge metadata.
spetrovic updated this object.Apr 30 2015, 2:13 AM
dsanders added a subscriber: atanasyan.

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
5797–5801 ↗(On Diff #24686)

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
5797–5801 ↗(On Diff #24686)

@spetrovic: I haven't seen a reply to this.

atanasyan added inline comments.May 19 2015, 8:28 AM
lib/CodeGen/ABIInfo.h
93 ↗(On Diff #24686)

Nit: Just for consistency let's:

  • define this routine in the cpp file
  • move the declaration closer to other virtual functions declared in this class
lib/CodeGen/TargetInfo.cpp
5800 ↗(On Diff #24686)

Nit: else-after-return violation.

return Ty->isUnsignedIntegerOrEnumerationType() && TySize == 32;

or

if (Ty->isUnsignedIntegerOrEnumerationType() && TySize == 32)
  return true;

return false;

And +1 for comment

spetrovic updated this revision to Diff 26235.May 21 2015, 6:49 AM
spetrovic set the repository for this revision to rL LLVM.

All comments addressed. One more test added with varargs, this patch also fixes vararg case (without patch unsigned int is ZEROEXT instead SIGNEXT).

atanasyan accepted this revision.May 22 2015, 1:46 AM
atanasyan edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 22 2015, 1:46 AM
dsanders accepted this revision.May 22 2015, 1:58 AM
dsanders edited edge metadata.

LGTM with a spelling nit in the new comment

lib/CodeGen/TargetInfo.cpp
5857 ↗(On Diff #26235)

Nit: Spelling of 'integers'

spetrovic updated this revision to Diff 26310.May 22 2015, 2:21 AM
spetrovic edited edge metadata.

Fixed spelling.

petarj accepted this revision.May 26 2015, 6:14 AM
petarj edited edge metadata.

lgtm

lib/CodeGen/CGCall.cpp
1592 ↗(On Diff #26310)

Nit: empty space after "if".

lib/CodeGen/TargetInfo.cpp
5858 ↗(On Diff #26310)

Nit: empty space after "if".

spetrovic updated this revision to Diff 26507.May 26 2015, 6:20 AM
spetrovic edited edge metadata.
This revision was automatically updated to reflect the committed changes.