Hi Alexey, Thanks for the review!
In http://reviews.llvm.org/D10753#195190, @ABataev wrote: > 1. Why you don't want to reuse an existing clang infrastructure for TLS > support? I think it would be much easier and portable solution to reuse an > existing code, rather than inventing a new one. I am afraid I don't understand exactly what you mean by TLS infrastructure. In my understanding, creating a TLS global variable will make each thread to have different storage for the variable but will not cause the Ctors/Dtors to run when a thread is spawn by the OpenMP runtime. Therefore the OpenMP runtime has to be aware of which Ctors/Dtors to use to a given variable even if it is declared as TLS so it can run them when it creates a thread. So from a codegen viewpoint, the step of registering the Dtors and Ctors is still required, only the cache creation and look-up can be skipped. That's the reason why I am reusing the existing code to do the registration. It is possible, however, that I am missing some feature in clang that would enable the execution of the Cotrs/Dtors when threads are spawn without intervention of any runtime library. If so, please give me a pointer and I will update the patch to use that instead. > 2. I don't like the idea that this feature is enabled only for a specific > platform. It requires a new class, some complex code changes etc. Instead I > prefer to have a driver/frontend option that makes the compiler to switch > codegen from cached version to TLS. If you want to set this feature on by > default for the PowerPC you can set this option in frontend if current > architecture is PowerPC. In this case we don't need an additional > CGOpenMPRuntime-based class. Done! Let me know if the option name okay. ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:996 @@ +995,3 @@ + RetVal = CtorCGF.Builder.CreatePointerCast( + TLSGV, CtorCGF.ConvertTypeForMem(CGM.getContext().VoidPtrTy)); + OrigVal = TLSGV; ---------------- ABataev wrote: > Use CtorCGF.VoidPtrTy rather than > CtorCGF.ConvertTypeForMem(CGM.getContext().VoidPtrTy) Done! ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1038 @@ +1037,3 @@ + OrigVal = DtorCGF.Builder.CreatePointerCast( + TLSGV, DtorCGF.ConvertTypeForMem(CGM.getContext().VoidPtrTy)); + } else { ---------------- ABataev wrote: > DtorCGF.VoidPtrTy Done! ================ Comment at: lib/CodeGen/CGOpenMPRuntime.h:355 @@ +354,3 @@ + //// variables. It is false by default. + bool useTLSForThreadPrivate; + ---------------- ABataev wrote: > Should start with an upper case letter, i.e. UseTLSForThreadPrivate Done! http://reviews.llvm.org/D10753 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
