rjmccall added inline comments.

================
Comment at: include/clang/Basic/Attr.td:1191
 
+def MipsLongCall : InheritableAttr, TargetSpecificAttr<TargetMips> {
+  let Spellings = [GCC<"long_call">, GCC<"far">, GCC<"near">];
----------------
Because this is used for all three attributes, I think you should call it 
something more general.  Perhaps MipsCallStyle?


================
Comment at: include/clang/Basic/Attr.td:1195
+  let Accessors = [Accessor<"longCall", [GCC<"long_call">, GCC<"far">]>,
+                   Accessor<"nearCall", [GCC<"near">]>];
+  let Documentation = [MipsLongCallDocs];
----------------
This is not the standard naming convention for accessors.  I suggest 
isLongCall() and isNearCall().


================
Comment at: include/clang/Basic/AttrDocs.td:1336
+if code compiled using ``-mlong-calls`` switch, it forces compiler to use
+the ``jal`` instruction to call the function.
+  }];
----------------
I suggest the following wording:

Clang supports the ``__attribute__((long_call))``, ``__attribute__((far))``, and
``__attribute__((near))`` attributes on MIPS targets.  These attributes may 
only be
added to function declarations and change the code generated by the compiler 
when
directly calling the function.  The ``near`` attribute allows calls to the 
function to
be made using the ``jal`` instruction, which requires the function to be 
defined in the
same 256MB segment as the caller.  The ``long_call`` and ``far`` attributes are
synonyms and require the use of a different call sequence that works regardless 
of
the distance between the functions.

These attributes take priority over command line switches such as 
``-mlong-calls``.


================
Comment at: lib/CodeGen/CGCall.cpp:1810
+        FuncAttrs.addAttribute("near-call");
+    }
+
----------------
You should really put this in TargetCodeGenInfo::setTargetAttributes.  Please 
just add a ForDefinition_t argument to that function and SetFunctionAttributes, 
then call setTargetAttributes from SetFunctionAttributes.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5955
+    handleSimpleAttribute<MipsLongCallAttr>(S, D, Attr);
+    break;
   case AttributeList::AT_AMDGPUFlatWorkGroupSize:
----------------
You need to check for conflicts between the different attributes, and please 
add a test for that.


Repository:
  rL LLVM

https://reviews.llvm.org/D35479



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to