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

[Eli Friedman]

On Mon, Nov 14, 2011 at 11:50 AM, Tony Linthicum <tlinth at codeaurora.org> wrote:
> Hi folks,
>
> These patches provide clang support for a new backend for Qualcomm's VLIW
> processor, Hexagon.  The backend itself has been submitted as two paches to
> the llvm-commits list.  As stated in that e-mail, this work is not mine
> alone and members of my team will likely also respond to any questions
> regarding this patch.
>
> I have sent these compressed via bzip2.  If that is inconvenient for anyone
> wishing to review them, please let me know and I can send them to you in
> another format.
>
> Thanks!

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

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

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

@@ -402,6 +403,7 @@ def fno_rtti : Flag<"-fno-rtti">, Group<f_Group>;
 def fno_short_enums : Flag<"-fno-short-enums">, Group<f_Group>;
 def fno_show_column : Flag<"-fno-show-column">, Group<f_Group>;
 def fno_show_source_location : Flag<"-fno-show-source-location">,
Group<f_Group>;
+def fno_simplify_div : Flag<"-fno-simplify-div">, Group<f_Group>;
 def fno_spell_checking : Flag<"-fno-spell-checking">, Group<f_Group>;
 def fno_stack_protector : Flag<"-fno-stack-protector">, Group<f_Group>;
 def fno_strict_aliasing : Flag<"-fno-strict-aliasing">, Group<f_Group>;

This looks a bit suspect, but I'll let you hash that out in the LLVM
side of the review.

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

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


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?


-Eli