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

[Eli Friedman]

On Mon, Nov 14, 2011 at 1:34 PM, Tony Linthicum <tlinth at codeaurora.org> wrote:
> 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.

Okay, that makes sense.

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

What?  I was just referring to the name of the option itself.

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

You aren't supposed to add trailing whitespace, but given that we
don't have a commit hook to stop it, it sometimes slips in anyway.

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

No, please don't submit a "patch to a patch".  You could probably
split this patch, though: one patch for the non-Driver changes without
the builtins, one patch for the builtins, and one patch for the
Driver.

-Eli