Change clang driver to pass -target-abi option

[vmedic]

Summary

Make changes to clang driver so it can pass -target-abi option to created jobs.

[dsanders]

It needs a testcase and I don't see where Opts.ABI is used.

By the way, please don't set custom access policies. There's no need for them.

tools/driver/cc1as_main.cpp

82

Not initialized in constructor.

Also, can you point me at the code that uses this variable? I can only see the code that sets it.

[atanasyan]

Not sure that the getTargetFeatures routine is a good place to configure the -target-abi option.

lib/Driver/Tools.cpp

1793

The getTargetFeatures routine called from the ClangAs::ConstructJob and from the Clang::ConstructJob. In the second case we add the -target-abi option twice. The first time when we call the getTargetFeatures and the second time when we call Clang::AddMIPSTargetArgs. I do not think it is a good idea.

[vmedic]

In D6091#8, @atanasyan wrote:

Not sure that the getTargetFeatures routine is a good place to configure the -target-abi option.

I was suspicious about this myself, but couldn't find the proper place to put the code in. Do you please have any idea where it would suite better?

[atanasyan]

Maybe create new method ClangAs::AddMIPSTargetArgs with -target-abi handling and call this method from ClangAs::ConstructJob. I think that code duplication is a minor problem here.

[vmedic]

A new method ClangAs::AddMIPSTargetArgs with -target-abi handling has been created. This method is called from ClangAs::ConstructJob to handle this particular task.

[atanasyan]

tools/driver/cc1as_main.cpp

192

Where do we use the value stored in the Opts.ABI?

[vmedic]

tools/driver/cc1as_main.cpp

192

I I understood correctly these options are passed to assembler when it is invoked with -cc1 option. Please correct me if I'm wrong.

[atanasyan]

And the patch still needs a test case.

tools/driver/cc1as_main.cpp

192

At the line cc1as_main.cpp:82 you add new field ABI to the AssemblerInvocation structure. At the line cc1as_main.cpp:191 you initialize this filed by the -target-abi arguments. But where is this new field used? For example the AssemblerInvocation::CPU filed is initialized at the line 186 and passed to the createMCSubtargetInfo at the line 357.

[vmedic]

Removed ABI field from AssemblerInvocation class. Added test cases for integrated assembler launch.

[atanasyan]

LGTM with the only notes

lib/Driver/Tools.cpp

1

Unnecessary change.

[echristo]

You'll want to remove the -target-feature bits of this code... honestly it shouldn't even compile at this point.

Actually this patch to the testcase doesn't appear to be against top of tree - can you rebase against that?

Thanks.

-eric

[vmedic]

Patch is rebased to the TOT. Passing -target-feature o32,n32,n64 options is disabled to remove warnings.

[vmedic]

Please review the updated patch.

[dsanders]

LGTM. The passing of -target-abi ought to be generic given that the backend handling is generic but this is still good as-is since it will silence the unknown feature warnings coming from the backend and fixing that is fairly urgent.

lib/Driver/Tools.cpp

4868

Nit: Indentation

4985

Nit: Indentation

lib/Driver/Tools.h

     ClangAs(const ToolChain &TC) : Tool("clang::as",
                                         "clang integrated assembler", TC,
                                         RF_Full) {}
-
+    void AddMIPSTargetArgs(const llvm::opt::ArgList &Args,
+                           llvm::opt::ArgStringList &CmdArgs) const;
     bool hasGoodDiagnostics() const override { return true; }
     bool hasIntegratedAssembler() const override { return false; }
     bool hasIntegratedCPP() const override { return false; }

lib/Driver/Tools.cpp

   return CLFallback.get();
 }
 
+void ClangAs::AddMIPSTargetArgs(const ArgList &Args,
+                              ArgStringList &CmdArgs) const {
+  StringRef CPUName;
+  StringRef ABIName;
+  const llvm::Triple &Triple = getToolChain().getTriple();
+  mips::getMipsCPUAndABI(Args, Triple, CPUName, ABIName);
+
+  CmdArgs.push_back("-target-abi");
+  CmdArgs.push_back(ABIName.data());
+}
+
 void ClangAs::ConstructJob(Compilation &C, const JobAction &JA,
                            const InputInfo &Output,
                            const InputInfoList &Inputs,

       getToolChain().getTriple().isOSLinux())
     SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
                    SplitDebugName(Args, Inputs));
+
+    // Add target specific flags.
+  switch(getToolChain().getArch()) {
+  default:
+    break;
+
+  case llvm::Triple::mips:
+  case llvm::Triple::mipsel:
+  case llvm::Triple::mips64:
+  case llvm::Triple::mips64el:
+    AddMIPSTargetArgs(Args, CmdArgs);
+    break;
+  }
 }
 
 void GnuTool::anchor() {}

tools/driver/cc1as_main.cpp

   /// be a list of strings starting with '+' or '-'.
   std::vector<std::string> Features;
 
+  /// If given, the name of the target ABI to use.
+  std::string ABI;
+
   /// @}
   /// @name Language Options
   /// @{

   Opts.Triple = llvm::Triple::normalize(Args->getLastArgValue(OPT_triple));
   Opts.CPU = Args->getLastArgValue(OPT_target_cpu);
   Opts.Features = Args->getAllArgValues(OPT_target_feature);
+  Opts.ABI = Args->getLastArgValue(OPT_target_abi);
 
   // Use the default target triple if unspecified.
   if (Opts.Triple.empty())