Add pointer types to global named register

[rengolin]

Summary

This patch adds support for pointer types in global named registers variables.
It'll be lowered as a pair of read/write_register and inttoptr/ptrtoint calls.
Also adds some early checks on types on SemaDecl to avoid the assert.

Tests changed accordingly. (PR19837)

[joerg]

Not directly related, but can you also check that the size of the integer type can be used for this register at the point of declaration? E.g. long long should be invalid on i386. GCC doesn't seem to check this at declaration time, but that feels bogus especially as it make the code dependent on the optimizer again.

[rnk]

Please add a test where the size of the integer type and the size of the register don't match as Joerg suggested.

[• rafael]

Please include a test with the new error.

I like Joeg and Reid suggestion of rejecting invalid sizes. For
example, a pointer should not go to eax on x86_64, but I think that
requires extra llvm support, no? If so, this patch LGTM, but please do
address their suggestion in a subsequent patch.

Thanks,
Rafael

[rengolin]

Hi Joerg,

Sorry for the delay.

I don't think it's currently possible to do the type size verification in Clang, and such a patch would not only be bigger than this, but completely off topic.

AFAIK, the function that checks for register names in Clang join *all* targets' registers into one list and match any of them. So, Clang accepts "sp" as a register names on x86 and "Q12" on ARMv4. The process that will check for real register lists per-target is in LLVM, and that's not even complete.

I can't promote/demote the integral type to the register size, since Clang has no knowledge if LLVM supports vector registers or if the target has more than one size of registers, or if the pointer type is actually the size of the register, etc, etc. Until Clang has that kind of knowledge, and verifies that the register name matches the target and know their sizes, it's when Clang will be able to check if the type size matches the register itself.

At least for now, LLVM has a fatal error on invalid register names and types.

cheers,
--renato

[joerg]

Yes, I am aware that it is unrelated. I saw the problem while trying out a few things and looking over the patch. Please don't let that inquire hold you back on this patch.

[rengolin]

Ok, I'll get this in, then, and have a look at the Clang register reporting next.

Thanks!

[rengolin]

Committed in r210274

include/clang/Basic/DiagnosticSemaKinds.td

     "invalid input constraint '%0' in asm">;
   def err_asm_invalid_type_in_input : Error<
     "invalid type %0 in asm input for constraint '%1'">;
   def err_asm_tying_incompatible_types : Error<
     "unsupported inline asm: input with type "
     "%diff{$ matching output with type $|}0,1">;
   def err_asm_incomplete_type : Error<"asm operand has incomplete type %0">;
   def err_asm_unknown_register_name : Error<"unknown register name '%0' in asm">;
+  def err_asm_bad_register_type : Error<"bad type for named register variable">;
   def err_asm_invalid_input_size : Error<
     "invalid input size for constraint '%0'">;
   def err_invalid_asm_cast_lvalue : Error<
     "invalid use of a cast in a inline asm context requiring an l-value: "
     "remove the cast or build with -fheinous-gnu-extensions">;
 
   def warn_asm_label_on_auto_decl : Warning<
     "ignored asm label '%0' on automatic variable">;

lib/CodeGen/CGExpr.cpp

   llvm::Value *MaskV = llvm::ConstantVector::get(Mask);
   Vec = Builder.CreateShuffleVector(Vec, llvm::UndefValue::get(Vec->getType()),
                                     MaskV);
   return RValue::get(Vec);
 }
 
 /// @brief Load of global gamed gegisters are always calls to intrinsics.
 RValue CodeGenFunction::EmitLoadOfGlobalRegLValue(LValue LV) {
-  assert(LV.getType()->isIntegerType() && "Bad type for register variable");
+  assert((LV.getType()->isIntegerType() || LV.getType()->isPointerType()) &&
+         "Bad type for register variable");
   llvm::MDNode *RegName = dyn_cast<llvm::MDNode>(LV.getGlobalReg());
   assert(RegName && "Register LValue is not metadata");
-  llvm::Type *Types[] = { CGM.getTypes().ConvertType(LV.getType()) };
+
+  // We accept integer and pointer types only
+  llvm::Type *OrigTy = CGM.getTypes().ConvertType(LV.getType());
+  llvm::Type *Ty = OrigTy;
+  if (OrigTy->isPointerTy())
+    Ty = CGM.getTypes().getDataLayout().getIntPtrType(OrigTy);
+  llvm::Type *Types[] = { Ty };
+
   llvm::Value *F = CGM.getIntrinsic(llvm::Intrinsic::read_register, Types);
-  llvm::Value* Call = Builder.CreateCall(F, RegName);
+  llvm::Value *Call = Builder.CreateCall(F, RegName);
+  if (OrigTy->isPointerTy())
+    Call = Builder.CreateIntToPtr(Call, OrigTy);
   return RValue::get(Call);
 }
 
 
 /// EmitStoreThroughLValue - Store the specified rvalue into the specified
 /// lvalue, where both are guaranteed to the have the same type, and that type
 /// is 'Ty'.
 void CodeGenFunction::EmitStoreThroughLValue(RValue Src, LValue Dst,

 
   llvm::StoreInst *Store = Builder.CreateStore(Vec, Dst.getExtVectorAddr(),
                                                Dst.isVolatileQualified());
   Store->setAlignment(Dst.getAlignment().getQuantity());
 }
 
 /// @brief Store of global named registers are always calls to intrinsics.
 void CodeGenFunction::EmitStoreThroughGlobalRegLValue(RValue Src, LValue Dst) {
-  assert(Dst.getType()->isIntegerType() && "Bad type for register variable");
+  assert((Dst.getType()->isIntegerType() || Dst.getType()->isPointerType()) &&
+         "Bad type for register variable");
   llvm::MDNode *RegName = dyn_cast<llvm::MDNode>(Dst.getGlobalReg());
   assert(RegName && "Register LValue is not metadata");
-  llvm::Type *Types[] = { CGM.getTypes().ConvertType(Dst.getType()) };
+
+  // We accept integer and pointer types only
+  llvm::Type *OrigTy = CGM.getTypes().ConvertType(Dst.getType());
+  llvm::Type *Ty = OrigTy;
+  if (OrigTy->isPointerTy())
+    Ty = CGM.getTypes().getDataLayout().getIntPtrType(OrigTy);
+  llvm::Type *Types[] = { Ty };
+
   llvm::Value *F = CGM.getIntrinsic(llvm::Intrinsic::write_register, Types);
   llvm::Value *Value = Src.getScalarVal();
+  if (OrigTy->isPointerTy())
+    Value = Builder.CreatePtrToInt(Value, Ty);
   Builder.CreateCall2(F, RegName, Value);
 }
 
 // setObjCGCLValueClass - sets class of the lvalue for the purpose of
 // generating write-barries API. It is currently a global, ivar,
 // or neither.
 static void setObjCGCLValueClass(const ASTContext &Ctx, const Expr *E,
                                  LValue &LV,

lib/Sema/SemaDecl.cpp

       case SC_PrivateExtern:
       case SC_OpenCLWorkGroupLocal:
         break;
       }
     } else if (SC == SC_Register) {
       // Global Named register
       if (!Context.getTargetInfo().isValidGCCRegisterName(Label))
         Diag(E->getExprLoc(), diag::err_asm_unknown_register_name) << Label;
+      if (!R->isIntegralType(Context) && !R->isPointerType()) {
+        Diag(D.getLocStart(), diag::err_asm_bad_register_type);
+        NewVD->setInvalidDecl(true);
+      }
     }
 
     NewVD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0),
                                                 Context, Label, 0));
   } else if (!ExtnameUndeclaredIdentifiers.empty()) {
     llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I =
       ExtnameUndeclaredIdentifiers.find(NewVD->getIdentifier());
     if (I != ExtnameUndeclaredIdentifiers.end()) {

test/CodeGen/named_reg_global.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
 // RUN: %clang_cc1 -triple arm64-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
 // RUN: %clang_cc1 -triple armv7-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
 
 // CHECK-NOT: @sp = common global
 register unsigned long current_stack_pointer asm("sp");
+struct p4_Thread {
+  struct {
+    int len;
+  } word;
+};
+// Testing pointer types as well
+register struct p4_Thread *p4TH asm("sp");
 
 // CHECK: define{{.*}} i[[bits:[0-9]+]] @get_stack_pointer_addr()
 // CHECK: [[ret:%[0-9]+]] = call i[[bits]] @llvm.read_register.i[[bits]](metadata !0)
 // CHECK: ret i[[bits]] [[ret]]
 unsigned long get_stack_pointer_addr() {
   return current_stack_pointer;
 }
 // CHECK: declare{{.*}} i[[bits]] @llvm.read_register.i[[bits]](metadata)
 
 // CHECK: define{{.*}} void @set_stack_pointer_addr(i[[bits]] %addr) #0 {
 // CHECK: [[sto:%[0-9]+]] = load i[[bits]]* %
 // CHECK: call void @llvm.write_register.i[[bits]](metadata !0, i[[bits]] [[sto]])
 // CHECK: ret void
 void set_stack_pointer_addr(unsigned long addr) {
   current_stack_pointer = addr;
 }
 // CHECK: declare{{.*}} void @llvm.write_register.i[[bits]](metadata, i[[bits]])
 
+// CHECK: define {{.*}}@fn1
+int fn1() {
+  return (*p4TH).word.len;
+}
+// CHECK: %[[regr:[0-9]+]] = call i[[bits]] @llvm.read_register.i[[bits]](metadata !0)
+// CHECK: inttoptr i[[bits]] %[[regr]] to %struct.p4_Thread*
+
+// CHECK: define {{.*}}@fn2
+void fn2(struct p4_Thread *val) {
+  p4TH = val;
+}
+// CHECK: %[[regw:[0-9]+]] = ptrtoint %struct.p4_Thread* %0 to i[[bits]]
+// CHECK: call void @llvm.write_register.i[[bits]](metadata !0, i[[bits]] %[[regw]])
+
 // CHECK: !llvm.named.register.sp = !{!0}
 // CHECK: !0 = metadata !{metadata !"sp"}