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

[Tony Linthicum]

Hi Eli,

Thanks for the quick responses!

On 11/14/2011 2:34 PM, Eli Friedman wrote:
>
> +BUILTIN(__builtin_HEXAGON_C2_and, "bii", "")
> +BUILTIN(__builtin_HEXAGON_C2_or, "bii", "")
> +BUILTIN(__builtin_HEXAGON_C2_xor, "bii", "")
>
> Do you really need builtins for these?  Fewer builtins means that
> standard LLVM optimizations generally work better.  Not a requirement,
> just something to think about.
>

There are a couple of reasons why we have the large number of builtins 
that we have.  First, they are auto-generated from an instruction 
database.  Second, some of our customers who use builtins for 
instructions that the compiler cannot otherwise generate will write all 
of that piece of code using builtins.  We can cull the list and are 
willing to do so if it is indeed a problem.  It might cause us some 
headaches with backward compatibility to our gcc compiler that we hope 
to replace with LLVM, so we'd prefer not to do it if it isn't strictly 
necessary.

> --- 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.
>

We will remove the naming convention backward compatibility option. 
Some of our customers use it, but we can maintain it locally if we need 
to carry it forward.

> --- 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.
>

Will do.

>
>
> Please don't include trailing whitespace changes to unrelated code in
> your patch.  This applies across the whole patch.
>

My apologies for that.  I made lots of changes to get our code within 
the 80 column limit and introduced trailing white spaces in the process. 
  I had a lot of them to fix and assumed all of them were mine.  I 
thought from the developer policy that removing them was a requirement. 
  Is that not so?

Others will be responding to some of your other points.  A general 
question, though, since the patches are so large.  When we address an 
issue and wish to resubmit the patch to the list, should we resubmit the 
whole thing or just a "patch to the patch" as it were.

Thanks again.

Tony


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.