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

[Kai Nacke]

Hi Michael!

I incorporated all your changes. The attached patch also includes the 
suggestions from Eli.

Please, could someone with knowledge of ISelLowering have a look at this 
patch? Thank you!

Regards
Kai

On 03.02.2012 02:56, Michael Spencer wrote:
> 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
>

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: tls-20120209.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120209/727cfd3b/attachment.ksh>