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

[spetrovic]

Summary

On MIPS unsigned int type should not be zero extended, it's sign extended now.

[dsanders]

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?

[spetrovic]

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.

[dsanders]

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]

In D9198#161802, @dsanders wrote:

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?

[spetrovic]

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

[dsanders]

In D9198#162000, @petarj wrote:

In D9198#161802, @dsanders wrote:

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.

In D9198#162018, @spetrovic wrote:

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

[petarj]

In D9198#162163, @dsanders wrote:

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

[spetrovic]

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.

[dsanders]

In D9198#162375, @petarj wrote:

In D9198#162163, @dsanders wrote:

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.

[dsanders]

In D9198#162493, @spetrovic wrote:

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]

ping

[dsanders]

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

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.

[spetrovic]

ping

[dsanders]

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

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

[atanasyan]

lib/CodeGen/ABIInfo.h

93

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

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]

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

[atanasyan]

LGTM

[dsanders]

LGTM with a spelling nit in the new comment

lib/CodeGen/TargetInfo.cpp

5799

Nit: Spelling of 'integers'

[spetrovic]

Fixed spelling.

[petarj]

lgtm

lib/CodeGen/CGCall.cpp

1592

Nit: empty space after "if".

lib/CodeGen/TargetInfo.cpp

5800

Nit: empty space after "if".

lib/CodeGen/ABIInfo.h

public:
     virtual bool isHomogeneousAggregateBaseType(QualType Ty) const;
 
     virtual bool isHomogeneousAggregateSmallEnough(const Type *Base,
                                                    uint64_t Members) const;
 
     bool isHomogeneousAggregate(QualType Ty, const Type *&Base,
                                 uint64_t &Members) const;
 
+    virtual bool shouldSignExtUnsignedType(QualType Ty) const {
+      return false;
+    }
+
   };
 }  // end namespace clang
 
 #endif

lib/CodeGen/CGCall.cpp

for (CGFunctionInfo::const_arg_iterator I = FI.arg_begin(),
 
     // 'restrict' -> 'noalias' is done in EmitFunctionProlog when we
     // have the corresponding parameter variable.  It doesn't make
     // sense to do it here because parameters are so messed up.
     switch (AI.getKind()) {
     case ABIArgInfo::Extend:
       if (ParamType->isSignedIntegerOrEnumerationType())
         Attrs.addAttribute(llvm::Attribute::SExt);
-      else if (ParamType->isUnsignedIntegerOrEnumerationType())
+      else if (ParamType->isUnsignedIntegerOrEnumerationType()) {
+        if(getTypes().getABIInfo().shouldSignExtUnsignedType(ParamType))
+          Attrs.addAttribute(llvm::Attribute::SExt);
+        else
           Attrs.addAttribute(llvm::Attribute::ZExt);
+      }
       // FALL THROUGH
     case ABIArgInfo::Direct:
       if (ArgNo == 0 && FI.isChainCall())
         Attrs.addAttribute(llvm::Attribute::Nest);
       else if (AI.getInReg())
         Attrs.addAttribute(llvm::Attribute::InReg);
       break;
 

lib/CodeGen/TargetInfo.cpp

MipsABIInfo(CodeGenTypes &CGT, bool _IsO32) :
     ABIInfo(CGT), IsO32(_IsO32), MinABIStackAlignInBytes(IsO32 ? 4 : 8),
     StackAlignInBytes(IsO32 ? 8 : 16) {}
 
   ABIArgInfo classifyReturnType(QualType RetTy) const;
   ABIArgInfo classifyArgumentType(QualType RetTy, uint64_t &Offset) const;
   void computeInfo(CGFunctionInfo &FI) const override;
   llvm::Value *EmitVAArg(llvm::Value *VAListAddr, QualType Ty,
                          CodeGenFunction &CGF) const override;
+  bool shouldSignExtUnsignedType(QualType Ty) const override;
 };
 
 class MIPSTargetCodeGenInfo : public TargetCodeGenInfo {
   unsigned SizeOfUnwindException;
 public:
   MIPSTargetCodeGenInfo(CodeGenTypes &CGT, bool IsO32)
     : TargetCodeGenInfo(new MipsABIInfo(CGT, IsO32)),
       SizeOfUnwindException(IsO32 ? 24 : 32) {}
llvm::Value* MipsABIInfo::EmitVAArg(llvm::Value *VAListAddr, QualType Ty,
   llvm::Value *NextAddr =
     Builder.CreateGEP(AlignedAddr, llvm::ConstantInt::get(IntTy, Offset),
                       "ap.next");
   Builder.CreateStore(NextAddr, VAListAddrAsBPP);
   
   return AddrTyped;
 }
 
+bool MipsABIInfo::shouldSignExtUnsignedType(QualType Ty) const {
+  int TySize = getContext().getTypeSize(Ty);
+  if(Ty->isUnsignedIntegerOrEnumerationType() && TySize == 32)
+    return true;
+  else
+    return false;
+}
+
 bool
 MIPSTargetCodeGenInfo::initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                                llvm::Value *Address) const {
   // This information comes from gcc's implementation, which seems to
   // as canonical as it gets.
 
   // Everything on MIPS is 4 bytes.  Double-precision FP registers
   // are aliased to pairs of single-precision FP registers.

test/CodeGen/atomics-inlining.c

 // MIPS32: = load atomic i8, i8* @c1 seq_cst
 // MIPS32: store atomic i8 {{.*}}, i8* @c1 seq_cst
 // MIPS32: = load atomic i16, i16* @s1 seq_cst
 // MIPS32: store atomic i16 {{.*}}, i16* @s1 seq_cst
 // MIPS32: = load atomic i32, i32* @i1 seq_cst
 // MIPS32: store atomic i32 {{.*}}, i32* @i1 seq_cst
 // MIPS32: call i64 @__atomic_load_8(i8* bitcast (i64* @ll1 to i8*)
 // MIPS32: call void @__atomic_store_8(i8* bitcast (i64* @ll1 to i8*), i64
-// MIPS32: call void @__atomic_load(i32 zeroext 100, i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a1, i32 0, i32 0), i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a2, i32 0, i32 0)
-// MIPS32: call void @__atomic_store(i32 zeroext 100, i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a1, i32 0, i32 0), i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a2, i32 0, i32 0)
+// MIPS32: call void @__atomic_load(i32 signext 100, i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a1, i32 0, i32 0), i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a2, i32 0, i32 0)
+// MIPS32: call void @__atomic_store(i32 signext 100, i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a1, i32 0, i32 0), i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a2, i32 0, i32 0)
 
 // MIPS64-LABEL: define void @test1
 // MIPS64: = load atomic i8, i8* @c1 seq_cst
 // MIPS64: store atomic i8 {{.*}}, i8* @c1 seq_cst
 // MIPS64: = load atomic i16, i16* @s1 seq_cst
 // MIPS64: store atomic i16 {{.*}}, i16* @s1 seq_cst
 // MIPS64: = load atomic i32, i32* @i1 seq_cst
 // MIPS64: store atomic i32 {{.*}}, i32* @i1 seq_cst
 // MIPS64: = load atomic i64, i64* @ll1 seq_cst
 // MIPS64: store atomic i64 {{.*}}, i64* @ll1 seq_cst
 // MIPS64: call void @__atomic_load(i64 zeroext 100, i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a1, i32 0, i32 0)
 // MIPS64: call void @__atomic_store(i64 zeroext 100, i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a1, i32 0, i32 0), i8* getelementptr inbounds ([100 x i8], [100 x i8]* @a2, i32 0, i32 0)
 }

test/CodeGen/mips-unsigned-extend.c

+// RUN: %clang -target mips64-unknown-linux -O0 -mabi=n64 -S -emit-llvm %s -o - | FileCheck %s -check-prefix=N64
+// RUN: %clang -target mips64-unknown-linux -O0 -mabi=n32 -S -emit-llvm %s -o - | FileCheck %s -check-prefix=N32
+// RUN: %clang -target mips-unknown-linux -O0 -mabi=o32 -S -emit-llvm %s -o - | FileCheck %s -check-prefix=O32 
+
+void foo(unsigned a) {
+}
+
+void foo1() {
+  unsigned f = 0xffffffe0;
+  foo(f);
+}
+
+// N64: call void @foo(i32 signext %0)
+// N32: call void @foo(i32 signext %0)
+// O32: call void @foo(i32 signext %0)
 No newline at end of file

test/CodeGenCXX/mips-size_t-ptrdiff_t.cpp

 // RUN: %clang_cc1 -x c++ -emit-llvm -triple=mips-unknown-linux-gnu < %s | FileCheck --check-prefix=O32 %s
 // RUN: %clang_cc1 -x c++ -emit-llvm -triple=mips64-unknown-linux-gnu -target-abi n32 < %s | FileCheck --check-prefix=N32 %s
 // RUN: %clang_cc1 -x c++ -emit-llvm -triple=mips64-unknown-linux-gnu -target-abi n64 < %s | FileCheck --check-prefix=N64 %s
 
 // Test that the size_t is correct for the ABI. It's not sufficient to be the
 // correct size, it must be the same type for correct name mangling.
 
 long *alloc_long() {
   long *rv = new long; // size_t is implicit in the new operator
   return rv;
 }
 // O32-LABEL: define i32* @_Z10alloc_longv()
-// O32: call noalias i8* @_Znwj(i32 zeroext 4)
+// O32: call noalias i8* @_Znwj(i32 signext 4)
 
 // N32-LABEL: define i32* @_Z10alloc_longv()
-// N32: call noalias i8* @_Znwj(i32 zeroext 4)
+// N32: call noalias i8* @_Znwj(i32 signext 4)
 
 // N64-LABEL: define i64* @_Z10alloc_longv()
 // N64: call noalias i8* @_Znwm(i64 zeroext 8)
 
 long *alloc_long_array() {
   long *rv = new long[2];
   return rv;
 }
 
 // O32-LABEL: define i32* @_Z16alloc_long_arrayv()
-// O32: call noalias i8* @_Znaj(i32 zeroext 8)
+// O32: call noalias i8* @_Znaj(i32 signext 8)
 
 // N32-LABEL: define i32* @_Z16alloc_long_arrayv()
-// N32: call noalias i8* @_Znaj(i32 zeroext 8)
+// N32: call noalias i8* @_Znaj(i32 signext 8)
 
 // N64-LABEL: define i64* @_Z16alloc_long_arrayv()
 // N64: call noalias i8* @_Znam(i64 zeroext 16)
 
 #include <stddef.h>
 
 void size_t_arg(size_t a) {
 }