Hi! I added 2 more tests and also refined an assert statement. Applies cleanly to r148473 now. Are there more comments on the code? Thank you!! Regards Kai On 01.01.2012 22:01, Eli Friedman wrote:> On Sun, Jan 1, 2012 at 10:44 AM, Kai<kai at redstar.de> wrote: >> Happy new year to all! >> >> 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++). >> Please review. Thanks! > > Rough review: looks like the patch is in the right direction; please > take out all tabs, please take out all commented-out code. > > -Eli-------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: tls-20120119.diff URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120119/89b04bb3/attachment.ksh>
On Thu, Jan 19, 2012 at 9:24 AM, Kai <kai at redstar.de> wrote:> Hi! > > I added 2 more tests and also refined an assert statement. Applies cleanly > to r148473 now. Are there more comments on the code? Thank you!!+ 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) && Just asserting "Inst.getOperand(ImmOp).isImm() || (Inst.getOperand(ImmOp).isExpr()" should be sufficient here; in theory, a wide variety of relocatable expressions are legal here, even if we don't actually generate code like that at the moment. I would prefer if someone more familiar with COFF MC stuff could take a look to make sure there isn't anything obviously wrong; otherwise, the patch looks good. -Eli
On 25.01.2012 22:38, Eli Friedman wrote:> On Thu, Jan 19, 2012 at 9:24 AM, Kai<kai at redstar.de> wrote: >> Hi! >> >> I added 2 more tests and also refined an assert statement. Applies cleanly >> to r148473 now. Are there more comments on the code? Thank you!! > + 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)&& > > Just asserting "Inst.getOperand(ImmOp).isImm() || > (Inst.getOperand(ImmOp).isExpr()" should be sufficient here; in > theory, a wide variety of relocatable expressions are legal here, even > if we don't actually generate code like that at the moment.I changed the assert as you proposed.> I would prefer if someone more familiar with COFF MC stuff could take > a look to make sure there isn't anything obviously wrong; otherwise, > the patch looks good.I think the "big change" is that I changed the relocation type from IMAGE_REL_AMD64_SREL32 to IMAGE_REL_AMD64_SECREL for 64bit systems. I believe that there are currently no uses of this relocation type, but I may be wrong here.> -Eli >Regards Kai -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: tls-20120126.diff URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120126/ba4dcac8/attachment.ksh>
Reasonably Related Threads
- [LLVMdev] [PATCH] TLS support for Windows 32+64bit
- [LLVMdev] LLC Bug x86 with thread local storage
- [LLVMdev] Adding fixups and relocations late in code generation
- [LLVMdev] LLC Bug x86 with thread local storage
- Printing PC-relative offsets - how to get the instruction length?