[cfe-commits] Patch: support for new backend for Hexagon processor

[Tony Linthicum]

On 11/14/2011 2:34 PM, Eli Friedman wrote:

>
> --- a/include/clang/Basic/LangOptions.def
> +++ b/include/clang/Basic/LangOptions.def
> @@ -127,6 +127,7 @@ LANGOPT(SinglePrecisionConstants , 1, 0, "treating
> double-precision floating poi
>   LANGOPT(FastRelaxedMath , 1, 0, "OpenCL fast relaxed math")
>   LANGOPT(DefaultFPContract , 1, 0, "FP_CONTRACT")
>   LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field type alignment")
> +LANGOPT(hexagon_qdsp6_compat , 1, 0, "hexagon-qdsp6 backward compatibility")
>
> Please use a consistent naming convention.
>

Sorry about the confusion in my previous response.  I thought you 
objected to the flag conceptually.  I have addressed this in the 
attached patch.  Please let me know if this is what you are looking for.

> --- a/include/clang/Driver/Options.td
> +++ b/include/clang/Driver/Options.td
> @@ -145,6 +145,7 @@ def D : JoinedOrSeparate<"-D">, Group<CompileOnly_Group>;
>   def E : Flag<"-E">, Flags<[DriverOption]>,
>     HelpText<"Only run the preprocessor">;
>   def F : JoinedOrSeparate<"-F">, Flags<[RenderJoined]>;
> +def G : Separate<"-G">, Flags<[DriverOption]>;
>   def H : Flag<"-H">;
>   def I_ : Flag<"-I-">, Group<I_Group>;
>   def I : JoinedOrSeparate<"-I">, Group<I_Group>;
>
> Can you use a more descriptive flag than "G"?  I understand not using
> "G" might not be possible due to compatibility constraints, but at
> least you could provide a more descriptive alternative name.
>

Added a new more descriptive flag as an alternative name.

>
>
> --- a/lib/Basic/Targets.cpp
> +++ b/lib/Basic/Targets.cpp
> @@ -898,7 +898,7 @@ namespace {
>         LongWidth = LongAlign = 64;
>         AddrSpaceMap =&PTXAddrSpaceMap;
>         // Define available target features
> -      // These must be defined in sorted order!
> +      // These must be defined in sorted order!
>
> Please don't include trailing whitespace changes to unrelated code in
> your patch.  This applies across the whole patch.
>

Fixed.

> +
> +Value *CodeGenFunction::EmitHexagonBuiltinExpr(unsigned BuiltinID,
> +                                             const CallExpr *E) {
> +  llvm::SmallVector<Value*, 4>  Ops;
> +
> +  for (unsigned i = 0, e = E->getNumArgs(); i != e; i++)
> +    Ops.push_back(EmitScalarExpr(E->getArg(i)));
> +
> +  Intrinsic::ID ID = Intrinsic::not_intrinsic;
> +
> +  switch (BuiltinID) {
> +  default: return 0;
> +
> +  case Hexagon::BI__builtin_HEXAGON_C2_cmpeq:
> +    ID = Intrinsic::hexagon_C2_cmpeq; break;
>
> [...]
>
> Do you actually need this?  You can mark an LLVM builtin as
> corresponding to a clang builtin in the LLVM TableGen bits using
> GCCBuiltinName (or something like that; I forget the exact name).
>

We will change this as you suggest.  Is it possible to accept the patch 
as is for now and we'll fix it later?

>
> Your changes in lib/Driver/ include a bunch of stuff which I don't
> really understand with little to no explanation.  What operating
> system are you trying to target?  Why are you using gcc to run the
> assembler and linker for a toolchain which you should have control
> over?
>

The OS is a proprietary RTOS, and we also use a proprietary libc.  Are 
you looking for more comments in the code to be added?

The assembler should be invoked directly.  We ran into a problem with 
the linker and are still using gcc for the moment.  We will fix this. 
Can the patch go in as is for now?

Thanks!

Tony

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.patch.bz2
Type: application/octet-stream
Size: 14358 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111115/11b1c6d9/attachment.obj>