[PATCH] Target-specific attributes

[Aaron Ballman]

On Sat, Jan 4, 2014 at 4:55 PM, Alp Toker <alp at nuanti.com> wrote:
> Hi Aaron,
>
> The identical-looking blocks look like a straight code-move so I haven't
> examined them closely (if there are any significant changes in there, do
> mention). Here's the rest..

You have it correct -- the code simply moved around, but has no
substantive changes.

>
>> +// Defines targets for target-specific attributes. The list of strings
>> should
>> +// specify architectures for which the target applies, based off the
>> ArchType
>> +// enumeration in Triple.h.
>> +class TargetArch<list<string> arches> {
>> +  list<string> Arches = arches;
>> +  list<string> OSes;
>> +}
>> +def TargetARM : TargetArch<["arm", "thumb"]>;
>> +def TargetMSP430 : TargetArch<["msp430"]>;
>> +def TargetX86 : TargetArch<["x86"]>;
>> +def TargetX86Win : TargetArch<["x86", "x86_64"]> {
>> +  let OSes = ["Win32", "MinGW32"];
>> +}
>> +def TargetMips : TargetArch<["mips", "mipsel"]>;
>> +
>
>
> Feels like it'd be more TableGen-ish to apply TargetArch<> directly to the
> instances without listing these utility wrappers. Maybe only provide the
> defs for TargetMips and TargetARM that add value? It's your call though if
> you prefer to keep it this way -- I don't have a strong opinion here.

I don't have a strong opinion either way, but I have a slight
preference for being more declarative like the patch is doing. The
rationale being: as people add more target-specific attributes, these
will be reused, which makes it easier to modify the targets (such as
adding thumb to arm, etc).

>
>> -def DLLExport : InheritableAttr, TargetSpecificAttr {
>> +def DLLExport : InheritableAttr, TargetSpecificAttr<TargetX86Win> {
>>    let Spellings = [Declspec<"dllexport">];
>>    let Subjects = SubjectList<[Function, Var]>;
>>  }
>>
>> -def DLLImport : InheritableAttr, TargetSpecificAttr {
>> +def DLLImport : InheritableAttr, TargetSpecificAttr<TargetX86Win> {
>>    let Spellings = [Declspec<"dllimport">];
>>    // Technically, the subjects for DllImport are Function and Var, but
>> there is
>>    // custom semantic handling required when MicrosoftExt is true.
>
>
> Any reason for dllimport / dllexport to be x86 target attributes?

Because they were x86 previously. This patch was going for feature
parity with the existing functionality.

> That quirk
> looks like a historical leftover that can be handled by the ["Win32",
> "MinGW32"] predicate alone thanks to your changes. MSDN documents them as
> being general language features not specific to x86 and I believe they apply
> to other Microsoft architectures equally.

That I could definitely believe, but should probably be a separate
patch. Good catch, though!

~Aaron