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

[Eli Friedman]

On Tue, Nov 15, 2011 at 3:15 PM, Tony Linthicum <tlinth at codeaurora.org> wrote:
> On 11/14/2011 2:34 PM, Eli Friedman wrote:
>
>>
>> --- 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.
>>
>
> Sorry about the confusion in my previous response.  I thought you objected
> to the flag conceptually.  I have addressed this in the attached patch.
>  Please let me know if this is what you are looking for.

Something more like HexagonQdsp6Compat would be better... but not a big deal.
>> +
>> +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).
>>
>
> We will change this as you suggest.  Is it possible to accept the patch as
> is for now and we'll fix it later?

Yes, that's okay.

>>
>> 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?
>>
>
> The OS is a proprietary RTOS, and we also use a proprietary libc.  Are you
> looking for more comments in the code to be added?

Oh... I see.  You should be changing Driver::GetHostInfo rather than
hijacking LinuxHostInfo for Hexagon; see how TCEHostInfo deals with it
for an example.

> The assembler should be invoked directly.  We ran into a problem with the
> linker and are still using gcc for the moment.  We will fix this. Can the
> patch go in as is for now?

Fine.


+  // Add Arch Information
+  Arg *A;
+  if ((A = getLastHexagonArchArg (Args))) {
+    if ((A->getOption().matches(options::OPT_march_EQ)) ||
+	(A->getOption().matches(options::OPT_mcpu_EQ))) {
+    llvm::StringRef WhichHexagon = A->getValue(Args);
+    if (WhichHexagon == "v2")

No tabs, please.

+//   if (Args.getLastArg(options::OPT_fprint_after_all)) {
+//     CmdArgs.push_back("-print-after-all");
+ //  }
+

I assume you didn't mean to leave this in the patch?


   // Pass the linker version in use.
-  if (Arg *A = Args.getLastArg(options::OPT_mlinker_version_EQ)) {
-    CmdArgs.push_back("-target-linker-version");
-    CmdArgs.push_back(A->getValue(Args));
+  if (getToolChain().getTriple().getArch() != llvm::Triple::hexagon){
+    if (Arg *A = Args.getLastArg(options::OPT_mlinker_version_EQ)) {
+      CmdArgs.push_back("-target-linker-version");
+      CmdArgs.push_back(A->getValue(Args));
+    }
   }

I don't understand the point of this change.

+void Clang::AddHexagonTargetArgs(const ArgList &Args,
+				 ArgStringList &CmdArgs) const {
+  llvm::Triple Triple = getToolChain().getTriple();
+
+  // FIXME: In this -march=v2 -mv3 will still select hexagonv2
+  // Set the CPU based on -march= and -mcpu=.
+  //  FIXED !! (See getLastHexagonArchArg above)

Is this comment necessary?

[...]
+  CmdArgs.push_back("-nobuiltininc");

I'm not sure why you need this...

+  CmdArgs.push_back("-mqdsp6-compat");

What is the point of this option if it's always on?

+  CmdArgs.push_back("-Wreturn-type");

What is this supposed to do?

+  if (!Args.hasArg(options::OPT_fno_short_enums))
+    CmdArgs.push_back("-fshort-enums");
+  if (!Args.hasArg(options::OPT_fuse_cxa_atexit))
+    CmdArgs.push_back("-fno-use-cxa-atexit");

If you need defaults for these, please move them to where we parse the options.

+  if (Args.getLastArg(options::OPT_fno_simplify_div)) {
+    CmdArgs.push_back ("-mllvm");
+    CmdArgs.push_back ("-disable_simplify_div");
+  }

This option, as far as I can tell, doesn't exist; what is it supposed to do?

-Eli