Re: [PATCH] D20709: Support ARM subtarget feature +long64
rsmith added a subscriber: rsmith. rsmith added a comment. The summary and content of this patch are very different. If you have an ABI variant that any language should be able to target, that should be controlled by the triple and related flags, not by a RenderScript language option. If you want to introduce RenderScript as a supported language in Clang, that may well be reasonable, but that's a very different discussion. Normally, we don't want to have ABI variants in clang that only a single language can target. There doesn't seem to be any fundamental reason why you couldn't compile C++ code for the RenderScript ABI, for instance, so tying the ABI to the language doesn't seem like the right approach. http://reviews.llvm.org/D20709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20709: Support ARM subtarget feature +long64
pirama updated this revision to Diff 59294. pirama removed a reviewer: rsmith. pirama added a comment. Herald added subscribers: danalbert, tberghammer. Add a RenderScript langopt and updated to change long's size and alignment in TargetInfo::adjust(). http://reviews.llvm.org/D20709 Files: include/clang/Basic/LangOptions.def include/clang/Driver/Types.def include/clang/Frontend/FrontendOptions.h lib/Basic/TargetInfo.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/FrontendActions.cpp test/CodeGen/renderscript.c Index: test/CodeGen/renderscript.c === --- /dev/null +++ test/CodeGen/renderscript.c @@ -0,0 +1,5 @@ +// RUN: %clang -target arm-linux-androideabi -DLONG_SIZE_AND_ALIGN=4 -fsyntax-only %s +// RUN: %clang -target arm-linux-androideabi -x renderscript -DLONG_SIZE_AND_ALIGN=8 -fsyntax-only %s + +_Static_assert(sizeof(long) == LONG_SIZE_AND_ALIGN, "sizeof long is wrong"); +_Static_assert(_Alignof(long) == LONG_SIZE_AND_ALIGN, "alignof long is wrong"); Index: lib/Frontend/FrontendActions.cpp === --- lib/Frontend/FrontendActions.cpp +++ lib/Frontend/FrontendActions.cpp @@ -735,6 +735,7 @@ case IK_PreprocessedObjCXX: case IK_AST: case IK_LLVM_IR: + case IK_Renderscript: // We can't do anything with these. return; } Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -1242,6 +1242,7 @@ .Case("objective-c++-header", IK_ObjCXX) .Cases("ast", "pcm", IK_AST) .Case("ir", IK_LLVM_IR) + .Case("renderscript", IK_Renderscript) .Default(IK_None); if (DashX == IK_None) Diags.Report(diag::err_drv_invalid_value) @@ -1445,6 +1446,9 @@ case IK_PreprocessedObjCXX: LangStd = LangStandard::lang_gnucxx98; break; +case IK_Renderscript: + LangStd = LangStandard::lang_c99; + break; } } @@ -1487,6 +1491,8 @@ Opts.CUDA = IK == IK_CUDA || IK == IK_PreprocessedCuda || LangStd == LangStandard::lang_cuda; + Opts.Renderscript = IK == IK_Renderscript; + // OpenCL and C++ both have bool, true, false keywords. Opts.Bool = Opts.OpenCL || Opts.CPlusPlus; Index: lib/Basic/TargetInfo.cpp === --- lib/Basic/TargetInfo.cpp +++ lib/Basic/TargetInfo.cpp @@ -318,6 +318,14 @@ FloatFormat = &llvm::APFloat::IEEEsingle; LongDoubleFormat = &llvm::APFloat::IEEEquad; } + + if (Opts.Renderscript) { +// In RenderScript, long types are always 8 bytes wide and aligned to +// 8-byte boundary. +if (PointerWidth == 32) { + LongWidth = LongAlign = 64; +} + } } bool TargetInfo::initFeatureMap( Index: include/clang/Frontend/FrontendOptions.h === --- include/clang/Frontend/FrontendOptions.h +++ include/clang/Frontend/FrontendOptions.h @@ -75,7 +75,8 @@ IK_CUDA, IK_PreprocessedCuda, IK_AST, - IK_LLVM_IR + IK_LLVM_IR, + IK_Renderscript }; Index: include/clang/Driver/Types.def === --- include/clang/Driver/Types.def +++ include/clang/Driver/Types.def @@ -53,6 +53,7 @@ TYPE("objective-c++-cpp-output", PP_ObjCXX,INVALID, "mii", "u") TYPE("objc++-cpp-output",PP_ObjCXX_Alias, INVALID, "mii", "u") TYPE("objective-c++",ObjCXX, PP_ObjCXX, "mm","u") +TYPE("renderscript", Renderscript, PP_C,"rs","u") // C family input files to precompile. TYPE("c-header-cpp-output", PP_CHeader, INVALID, "i", "p") Index: include/clang/Basic/LangOptions.def === --- include/clang/Basic/LangOptions.def +++ include/clang/Basic/LangOptions.def @@ -185,6 +185,7 @@ LANGOPT(OpenMP, 1, 0, "OpenMP support") LANGOPT(OpenMPUseTLS , 1, 0, "Use TLS for threadprivates or runtime calls") LANGOPT(OpenMPIsDevice, 1, 0, "Generate code only for OpenMP target device") +LANGOPT(Renderscript , 1, 0, "RenderScript") LANGOPT(CUDAIsDevice , 1, 0, "compiling for CUDA device") LANGOPT(CUDAAllowVariadicFunctions, 1, 0, "allowing variadic functions in CUDA device code") ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20709: Support ARM subtarget feature +long64
pirama added a reviewer: rsmith. pirama added a comment. Adding Richard to reviewers as the planned direction of this patch directly relates to the Frontend. http://reviews.llvm.org/D20709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20709: Support ARM subtarget feature +long64
pirama added a comment. I looked at how LangOpts and TargetInfo are handled and found a way we can achieve 64-bit longs without a Target feature (and restrict it just to RenderScript!). This would be via TargetInfo::adjust(). I'll abandon the sibling patch to LLVM as we should be able to do this just in Clang. http://reviews.llvm.org/D20709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20709: Support ARM subtarget feature +long64
rengolin added a comment. In http://reviews.llvm.org/D20709#443950, @srhines wrote: > Correct. This is only used by RenderScript, and unfortunately can't be done > any differently. We had hoped to just predicate this with our own LangOpt > (the patch that adds the RenderScript LangOpt is coming soon), but > unfortunately, at the point where LangOpts are being parsed, the target > machine already has the size and alignment of long specified (and it can't > change). Hi Stephen, GCC uses -mabi=ilp32 or -mabi=lp64 for AArch64 to change the ABI, you could do the same on Renderscript. If this is how Renderscript *always* operate, than having a "renderscript" ABI would make even more sense. cheers, --renato http://reviews.llvm.org/D20709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20709: Support ARM subtarget feature +long64
srhines added a comment. In http://reviews.llvm.org/D20709#443536, @kristof.beyls wrote: > Hi Pirama, > > My understanding is that this introduces (yet another) ARM 32 bit ABI variant > - in this case with longs being 64 bit. > My understanding is also that this is the ABI that is used in Renderscript, > and this patch helps to remove local patches that currently live in the > Renderscript toolchain downstream only. Correct. This is only used by RenderScript, and unfortunately can't be done any differently. We had hoped to just predicate this with our own LangOpt (the patch that adds the RenderScript LangOpt is coming soon), but unfortunately, at the point where LangOpts are being parsed, the target machine already has the size and alignment of long specified (and it can't change). > I think it's good not to need downstream patches for a Renderscript toolchain. Same here, although these patches seem so small that I don't want to introduce too much trouble in upstream. If there isn't a nice way to clean this up, I would be fine with this amount of divergence, since it really doesn't impact our ability to update LLVM (and again, the feature is only used by RenderScript). > In effect, this introduces another abi variant. > I'm wondering if the abi variant should be controlled using a triple, just > like e.g. the hard float vs. soft float abi variants also get controlled via > a triple? We can't unfortunately control this via a separate triple because RenderScript has (and always) will use the ARM triple. This really only affects the frontend for RenderScript, since RS doesn't allow the full libc runtime. That is why the LLVM side of this patch has no tests (because there is nothing there, and we only need the option recognized or the backend generates warnings about an unsupported flag - perhaps there is a better way to suppress that, but at the time that I fixed this years ago, I think my only option was to add it in both places). > Tim, do you happen to have insights on this? > > Thanks, > > Kristof http://reviews.llvm.org/D20709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20709: Support ARM subtarget feature +long64
kristof.beyls added a comment. Hi Pirama, My understanding is that this introduces (yet another) ARM 32 bit ABI variant - in this case with longs being 64 bit. My understanding is also that this is the ABI that is used in Renderscript, and this patch helps to remove local patches that currently live in the Renderscript toolchain downstream only. I think it's good not to need downstream patches for a Renderscript toolchain. In effect, this introduces another abi variant. I'm wondering if the abi variant should be controlled using a triple, just like e.g. the hard float vs. soft float abi variants also get controlled via a triple? Tim, do you happen to have insights on this? Thanks, Kristof http://reviews.llvm.org/D20709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits