[llvm-commits] [PATCH] TLS support for Windows 32+64bit

[Michael Spencer]

On Tue, Jan 24, 2012 at 12:24 PM, Kai <kai at redstar.de> wrote:
> Hello,
>
> the attached patch adds TLS support for x86_64-pc-win32 and x86-pc-win32.
> Implemented is the implicit TLS model (__declspec(thread) in Visual C++).
> This is one of the missing pieces for the Win64 port of
> LDC (the LLVM based D compiler). It should also be useful for implementing
> the Microsoft specific TLS extension in Clang.
>
> Please review and commit if acceptable. Thank you very much!
>
> Regards
> Kai

The patch looks good, however, I am not familiar enough with
ISelLowering to give approval. I've tested it and the behavior matches
what VC++ generates.

There are some LLVM style issues that I've noted below.

@@ -231,6 +232,12 @@ EmitImmediate(const MCOperand &DispOp, unsigned
Size, MCFixupKind FixupKind,
       if (Kind == GOT_Normal)
         ImmOffset = CurByte;
     }
+    else if (Expr->getKind() == MCExpr::SymbolRef) {
+      const MCSymbolRefExpr *Ref = static_cast<const MCSymbolRefExpr*>(Expr);
+      if (Ref->getKind() == MCSymbolRefExpr::VK_SECREL) {
+        FixupKind = MCFixupKind(FK_SecRel_4);
+      }
+    }

There should not be a new line between the } and else if.

+    // If GV is an alias then use the aliasee for determining
+    // thread-localness.
+    if (const GlobalAlias *GA = dyn_cast<GlobalAlias>(GV))
+      GV = GA->resolveAliasedGlobal(false);
+    DebugLoc dl = GA->getDebugLoc();
+   SDValue Chain = DAG.getEntryNode();

The indentation drops by one space here to the end of the block.

+   if (Subtarget->is64Bit())
+       IDX = DAG.getExtLoad(ISD::ZEXTLOAD, dl, getPointerTy(), Chain,
+                            IDX, MachinePointerInfo(), MVT::i32,
+                            false, false, 0);
+   else
+       IDX = DAG.getLoad(getPointerTy(), dl, Chain, IDX, MachinePointerInfo(),
+                         false, false, false, 0);

These are indented too far. We use two space indentation.

-  assert(Inst.getOperand(0).isReg() && Inst.getOperand(ImmOp).isImm() &&
+  assert(Inst.getOperand(0).isReg() &&
+	     (Inst.getOperand(ImmOp).isImm() ||
+		 (Inst.getOperand(ImmOp).isExpr() &&
+		  Inst.getOperand(ImmOp).getExpr()->getKind() == MCExpr::SymbolRef) &&
+		  static_cast<const
MCSymbolRefExpr*>(Inst.getOperand(ImmOp).getExpr())->getKind() ==
MCSymbolRefExpr::VK_SECREL) &&

This is over 80 columns. It also contains tabs.

+; RUN: llc < %s -march=x86-64 -mtriple=x86_64-pc-win32 > %t3
+; RUN: grep {movl	_tls_index(%rip), %eax} %t3
+; RUN: grep {movabsq	\$i at SECREL, %rcx} %t3
+; RUN: llc < %s -march=x86 -mtriple=x86-pc-win32 > %t4
+; RUN: grep {movl	__tls_index, %eax} %t4
+; RUN: grep {movl	%fs:__tls_array, %ecx} %t4
+; RUN: grep {movl	_i at SECREL(%eax), %eax} %t4

You should use FileCheck instead of grep for new tests.

Thanks for working on this. I'm excited to see more work on Windows codegen :)

- Michael Spencer