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

   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
     getMIPSTargetFeatures(D, Triple, Args, Features);
+    if (ForAS) {
+      StringRef CPUName;
+      StringRef ABIName;
+      mips::getMipsCPUAndABI(Args, Triple, CPUName, ABIName);
+    
+      CmdArgs.push_back("-target-abi");
+      CmdArgs.push_back(ABIName.data());
+    }
     break;
 
   case llvm::Triple::arm:

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())