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 ↗

(On Diff #15711)

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 ↗

(On Diff #16140)

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

[vmedic]

tools/driver/cc1as_main.cpp

192 ↗

(On Diff #16140)

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 ↗

(On Diff #16140)

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/Basic/Targets.cpp

         .Case("mips32r2", IsMips32)
         .Case("mips32r6", IsMips32)
         .Case("mips64", true)
         .Case("mips64r2", true)
         .Case("mips64r6", true)
         .Case("octeon", true)
         .Default(false);
   }
   const std::string& getCPU() const { return CPU; }
   void getDefaultFeatures(llvm::StringMap<bool> &Features) const override {
-    // The backend enables certain ABI's by default according to the
-    // architecture.
-    // Disable both possible defaults so that we don't end up with multiple
-    // ABI's selected and trigger an assertion.
-    Features["o32"] = false;
-    Features["n64"] = false;
-
-    Features[ABI] = true;
     if (CPU == "octeon")
       Features["mips64r2"] = Features["cnmips"] = true;
     else
       Features[CPU] = true;
   }
 
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override {
     Builder.defineMacro("__mips__");
     Builder.defineMacro("_mips");

lib/Driver/Tools.h

                       const llvm::opt::ArgList &TCArgs,
                       const char *LinkingOutput) const override;
   };
 
   /// \brief Clang integrated assembler tool.
   class LLVM_LIBRARY_VISIBILITY ClangAs : public Tool {
   public:
     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; }
 
     void ConstructJob(Compilation &C, const JobAction &JA,
                       const InputInfo &Output, const InputInfoList &Inputs,
                       const llvm::opt::ArgList &TCArgs,
                       const char *LinkingOutput) const override;
   };
 

lib/Driver/Tools.cpp

 }
 
 static void getMIPSTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                                   const ArgList &Args,
                                   std::vector<const char *> &Features) {
   StringRef CPUName;
   StringRef ABIName;
   mips::getMipsCPUAndABI(Args, Triple, CPUName, ABIName);
   ABIName = getGnuCompatibleMipsABIName(ABIName);
 
-  // Always override the backend's default ABI.
-  std::string ABIFeature = llvm::StringSwitch<StringRef>(ABIName)
-                               .Case("32", "+o32")
-                               .Case("n32", "+n32")
-                               .Case("64", "+n64")
-                               .Case("eabi", "+eabi")
-                               .Default(("+" + ABIName).str());
-  Features.push_back("-o32");
-  Features.push_back("-n64");
-  Features.push_back(Args.MakeArgString(ABIFeature));
-
   AddTargetFeature(Args, Features, options::OPT_mno_abicalls,
                    options::OPT_mabicalls, "noabicalls");
 
   StringRef FloatABI = getMipsFloatABI(D, Args);
   if (FloatABI == "soft") {
     // FIXME: Note, this is a hack. We need to pass the selected float
     // mode to the MipsTargetInfoBase to define appropriate macros there.
     // Now it is the only method.
     Features.push_back("+soft-float");
   }

       CmdArgs.push_back("msvc");
   }
 }
 
 visualstudio::Compile *Clang::getCLFallback() const {
   if (!CLFallback)
     CLFallback.reset(new visualstudio::Compile(getToolChain()));
   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,
                            const ArgList &Args,
                            const char *LinkingOutput) const {
   ArgStringList CmdArgs;
 
   assert(Inputs.size() == 1 && "Unexpected number of inputs.");
   const InputInfo &Input = Inputs[0];
 

       EscapeSpacesAndBackslashes(OriginalArgs[i], EscapedArg);
       Flags += " ";
       Flags += EscapedArg;
     }
     CmdArgs.push_back("-dwarf-debug-flags");
     CmdArgs.push_back(Args.MakeArgString(Flags.str()));
   }
 
   // FIXME: Add -static support, once we have it.
 
+    // 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;
+  }
+
   // Consume all the warning flags. Usually this would be handled more
   // gracefully by -cc1 (warning about unknown warning flags, etc) but -cc1as
   // doesn't handle that so rather than warning about unused flags that are
   // actually used, we'll lie by omission instead.
   // FIXME: Stop lying and consume only the appropriate driver flags
   for (arg_iterator it = Args.filtered_begin(options::OPT_W_Group),
                     ie = Args.filtered_end();
        it != ie; ++it)
     (*it)->claim();
 

test/Driver/mips-integrated-as.s

 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-O32 %s
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mabi=32 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-O32 %s
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mabi=o32 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-O32 %s
 // ABI-O32: -cc1as
-// ABI-O32: "-target-feature" "-n64"
-// ABI-O32: "-target-feature" "+o32"
+// ABI-O32: "-target-abi" "o32"
 
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mabi=eabi 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-EABI32 %s
 // ABI-EABI32: -cc1as
-// ABI-EABI32: "-target-feature" "-o32"
-// ABI-EABI32: "-target-feature" "-n64"
-// ABI-EABI32: "-target-feature" "+eabi"
+// ABI-EABI32: "-target-abi" "eabi"
 
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mips64 -mabi=n32 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-N32 %s
 // RUN: %clang -target mips64-linux-gnu -### -fintegrated-as -c %s -mabi=n32 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-N32 %s
 // ABI-N32: -cc1as
-// ABI-N32: "-target-feature" "-o32"
-// ABI-N32: "-target-feature" "-n64"
-// ABI-N32: "-target-feature" "+n32"
+// ABI-N32: "-target-abi" "n32"
 
 // FIXME: We should also test '-target mips-linux-gnu -mips64' defaults to the
 //        default 64-bit ABI (N64 but GCC uses N32). It currently selects O32
 //        because of the triple.
 // RUN: %clang -target mips64-linux-gnu -### -fintegrated-as -c %s -mips64 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-N64 %s
 //
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mips64 -mabi=64 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-N64 %s
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mips64 -mabi=n64 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-N64 %s
 // RUN: %clang -target mips64-linux-gnu -### -fintegrated-as -c %s -mips64 -mabi=64 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-N64 %s
 // RUN: %clang -target mips64-linux-gnu -### -fintegrated-as -c %s -mips64 -mabi=n64 2>&1 | \
 // RUN:   FileCheck -check-prefix=ABI-N64 %s
 // ABI-N64: -cc1as
-// ABI-N64: "-target-feature" "-o32"
-// ABI-N64: "-target-feature" "+n64"
+// ABI-N64: "-target-abi" "n64"
 
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -msoft-float 2>&1 | \
 // RUN:   FileCheck -check-prefix=SOFTFLOAT %s
 // SOFTFLOAT: -cc1as
 // SOFTFLOAT: "-target-feature" "+soft-float"
 
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=HARDFLOAT %s
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mhard-float 2>&1 | \
 // RUN:   FileCheck -check-prefix=HARDFLOAT %s