Handle -mlong-calls on Hexagon

[kparzysz]

Summary

Move long-calls to m_Group, add handling of that option to the Hexagon target.

[echristo]

Why isn't the existing +long-calls handling good enough (other than making it a generic option rather than arm specific)? You don't seem to be using any of the code in Targets.cpp to do anything.

Also a couple of random comments inline on the patch.

-eric

lib/Basic/Targets.cpp

6263–6266

This seems to be overly complex.

6282–6288

This appears to be unrelated?

[kparzysz]

It likely is sufficient. The code in Targets.cpp adds checking for "long-calls" feature in hasFeature, but it's not explicitly used anywhere. The rest of the changes are mostly to clean up the handling of feature strings.

[echristo]

Go ahead and split out your cleanups into another patch and remove the custom handling in the hexagon target then.

Thanks!

-eric

[kparzysz]

Removed the cleanup part.

[kparzysz]

Ping?

[echristo]

You haven't removed the custom handling?

-eric

[kparzysz]

In D22766#505512, @echristo wrote:

You haven't removed the custom handling?

The one from the driver? If I remove it, it won't be passed to the compiler (and the testcase will fail).

I forgot to move the no-long-calls from the ARM group to the m group, so I will need to post another patch, but I'd like to know what specific changes you are expecting. I think I was wrong in my earlier comment about the pre-existing handling of long-calls, since it only happens in the ARM target.

[kparzysz]

Moving mno_long_calls to m_Group as well.

I'm hoping this is all that's needed: what I understand as "custom handling" is still needed, since this option is only valid for ARM and Hexagon. The Hexagon way of handling target features will be cleaned up in a subsequent patch.

[kparzysz]

Ping.

[compnerd]

include/clang/Driver/Options.td

1383

It seems a bit weird to have target specific descriptions. AFAIK, the behavior is the same on all the targets: it generates branches which have full addressability (ARC, ARM, BlackFin, Epiphany, MIPS, PPC all support this option, probably amongst other architectures). This would easily grow unwieldily if we try to have target specific descriptions. We support at least ARM, MIPS, PPC, and now Hexagon. Why not make the description generic, something like:

Generate branches with extended addressability, usually via indirect jumps.

[kparzysz]

include/clang/Driver/Options.td

1383

The generic description sounds good to me.

At the same time, I tried -mlong-calls and -mno-long-calls on a trivial C procedure and for both MIPS and PPC/PPC64 I got a warning: "argument unused during compilation: '-mlong-calls'". The generated code for both does not change with -mlong-calls or with -mno-long-calls. What kind of support are you talking about?

Here's the code I tried:

void bar();

void foo() {
  bar();
}

$ clang -target powerpc64 -mno-long-calls -S call.c -O2
clang-4.0: warning: argument unused during compilation: '-mno-long-calls'

[kparzysz]

Changed the option description to be nont

[kparzysz]

Ping.

[compnerd]

That is a bug in the LLVM implementation. The functionality is supported on other (i.e. gcc) compilers.

include/clang/Driver/Options.td

 def mabi_EQ : Joined<["-"], "mabi=">, Group<m_Group>;
 def miamcu : Flag<["-"], "miamcu">, Group<m_Group>, Flags<[DriverOption, CoreOption]>,
   HelpText<"Use Intel MCU ABI">;
 def mno_iamcu : Flag<["-"], "mno-iamcu">, Group<m_Group>, Flags<[DriverOption, CoreOption]>;
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, Group<clang_ignored_m_Group>;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, Group<clang_ignored_m_Group>;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, Group<clang_ignored_m_Group>;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, Group<clang_ignored_m_Group>;
+def mlong_calls : Flag<["-"], "mlong-calls">, Group<m_Group>,
+  HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
+def mno_long_calls : Flag<["-"], "mno-long-calls">, Group<m_Group>,
+  HelpText<"Restore the default behaviour of not generating long calls">;
 def mtvos_version_min_EQ : Joined<["-"], "mtvos-version-min=">, Group<m_Group>;
 def mappletvos_version_min_EQ : Joined<["-"], "mappletvos-version-min=">, Alias<mtvos_version_min_EQ>;
 def mtvos_simulator_version_min_EQ : Joined<["-"], "mtvos-simulator-version-min=">, Alias<mtvos_version_min_EQ>;
 def mappletvsimulator_version_min_EQ : Joined<["-"], "mappletvsimulator-version-min=">, Alias<mtvos_version_min_EQ>;
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, Group<m_Group>;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], "mwatchos-simulator-version-min=">, Alias<mwatchos_version_min_EQ>;
 def mwatchsimulator_version_min_EQ : Joined<["-"], "mwatchsimulator-version-min=">, Alias<mwatchos_version_min_EQ>;
 def march_EQ : Joined<["-"], "march=">, Group<m_Group>;

 def ffixed_r9 : Flag<["-"], "ffixed-r9">, Group<m_arm_Features_Group>,
   HelpText<"Reserve the r9 register (ARM only)">;
 def mno_movt : Flag<["-"], "mno-movt">, Group<m_arm_Features_Group>,
   HelpText<"Disallow use of movt/movw pairs (ARM only)">;
 def mcrc : Flag<["-"], "mcrc">, Group<m_arm_Features_Group>,
   HelpText<"Allow use of CRC instructions (ARM only)">;
 def mnocrc : Flag<["-"], "mnocrc">, Group<m_arm_Features_Group>,
   HelpText<"Disallow use of CRC instructions (ARM only)">;
-def mlong_calls : Flag<["-"], "mlong-calls">, Group<m_arm_Features_Group>,
-  HelpText<"Generate an indirect jump to enable jumps further than 64M">;
-def mno_long_calls : Flag<["-"], "mno-long-calls">, Group<m_arm_Features_Group>,
-  HelpText<"Restore the default behaviour of not generating long calls">;
 
 def mgeneral_regs_only : Flag<["-"], "mgeneral-regs-only">, Group<m_aarch64_Features_Group>,
   HelpText<"Generate code which only uses the general purpose registers (AArch64 only)">;
 
 def mfix_cortex_a53_835769 : Flag<["-"], "mfix-cortex-a53-835769">,
   Group<m_aarch64_Features_Group>,
   HelpText<"Workaround Cortex-A53 erratum 835769 (AArch64 only)">;
 def mno_fix_cortex_a53_835769 : Flag<["-"], "mno-fix-cortex-a53-835769">,

lib/Basic/Targets.cpp

 
 // Hexagon abstract base class
 class HexagonTargetInfo : public TargetInfo {
   static const Builtin::Info BuiltinInfo[];
   static const char * const GCCRegNames[];
   static const TargetInfo::GCCRegAlias GCCRegAliases[];
   std::string CPU;
   bool HasHVX, HasHVXDouble;
+  bool UseLongCalls;
 
 public:
   HexagonTargetInfo(const llvm::Triple &Triple, const TargetOptions &)
       : TargetInfo(Triple) {
     BigEndian = false;
     // Specify the vector alignment explicitly. For v512x1, the calculated
     // alignment would be 512*alignment(i1), which is 512 bytes, instead of
     // the required minimum of 64 bytes.
     resetDataLayout("e-m:e-p:32:32:32-a:0-n16:32-"
         "i64:64:64-i32:32:32-i16:16:16-i1:8:8-f32:32:32-f64:64:64-"
         "v32:32:32-v64:64:64-v512:512:512-v1024:1024:1024-v2048:2048:2048");
     SizeType    = UnsignedInt;
     PtrDiffType = SignedInt;
     IntPtrType  = SignedInt;
 
     // {} in inline assembly are packet specifiers, not assembly variant
     // specifiers.
     NoAsmVariants = true;
 
     LargeArrayMinWidth = 64;
     LargeArrayAlign = 64;
     UseBitFieldTypeAlignment = true;
     ZeroLengthBitfieldBoundary = 32;
     HasHVX = HasHVXDouble = false;
+    UseLongCalls = false;
   }
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override {
     return llvm::makeArrayRef(BuiltinInfo,
                          clang::Hexagon::LastTSBuiltin-Builtin::FirstTSBuiltin);
   }
 
   bool validateAsmConstraint(const char *&Name,
public:
 
   bool isCLZForZeroUndef() const override { return false; }
 
   bool hasFeature(StringRef Feature) const override {
     return llvm::StringSwitch<bool>(Feature)
       .Case("hexagon", true)
       .Case("hvx", HasHVX)
       .Case("hvx-double", HasHVXDouble)
+      .Case("long-calls", UseLongCalls)
       .Default(false);
   }
 
   bool initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
         StringRef CPU, const std::vector<std::string> &FeaturesVec)
         const override;
 
   bool handleTargetFeatures(std::vector<std::string> &Features,
for (auto &F : Features) {
     if (F == "+hvx")
       HasHVX = true;
     else if (F == "-hvx")
       HasHVX = HasHVXDouble = false;
     else if (F == "+hvx-double")
       HasHVX = HasHVXDouble = true;
     else if (F == "-hvx-double")
       HasHVXDouble = false;
+
+    if (F == "+long-calls")
+      UseLongCalls = true;
   }
   return true;
 }
 
 bool HexagonTargetInfo::initFeatureMap(llvm::StringMap<bool> &Features,
       DiagnosticsEngine &Diags, StringRef CPU,
       const std::vector<std::string> &FeaturesVec) const {
   // Default for v60: -hvx, -hvx-double.
   Features["hvx"] = false;
   Features["hvx-double"] = false;
+  Features["long-calls"] = false;
 
   return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
 }
 
-
 const char *const HexagonTargetInfo::GCCRegNames[] = {
   "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
   "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",
   "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
   "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
   "p0", "p1", "p2", "p3",
   "sa0", "lc0", "sa1", "lc1", "m0", "m1", "usr", "ugp"
 };
 
 ArrayRef<const char*> HexagonTargetInfo::getGCCRegNames() const {
   return llvm::makeArrayRef(GCCRegNames);
 }
 
 const TargetInfo::GCCRegAlias HexagonTargetInfo::GCCRegAliases[] = {
   { { "sp" }, "r29" },
   { { "fp" }, "r30" },
   { { "lr" }, "r31" },

lib/Driver/Tools.cpp

if (A->getOption().matches(options::OPT_mno_unaligned_access))
       Features.push_back("+strict-align");
 
   if (Args.hasArg(options::OPT_ffixed_x18))
     Features.push_back("+reserve-x18");
 }
 
 static void getHexagonTargetFeatures(const ArgList &Args,
                                      std::vector<const char *> &Features) {
-  bool HasHVX = false, HasHVXD = false;
+  bool HasHVX = false, HasHVXD = false, UseLongCalls = false;
 
   // FIXME: This should be able to use handleTargetFeaturesGroup except it is
   // doing dependent option handling here rather than in initFeatureMap or a
   // similar handler.
   for (auto &A : Args) {
     auto &Opt = A->getOption();
     if (Opt.matches(options::OPT_mhexagon_hvx))
       HasHVX = true;
     else if (Opt.matches(options::OPT_mno_hexagon_hvx))
       HasHVXD = HasHVX = false;
     else if (Opt.matches(options::OPT_mhexagon_hvx_double))
       HasHVXD = HasHVX = true;
     else if (Opt.matches(options::OPT_mno_hexagon_hvx_double))
       HasHVXD = false;
+    else if (Opt.matches(options::OPT_mlong_calls))
+      UseLongCalls = true;
+    else if (Opt.matches(options::OPT_mno_long_calls))
+      UseLongCalls = false;
     else
       continue;
     A->claim();
   }
 
   Features.push_back(HasHVX  ? "+hvx" : "-hvx");
   Features.push_back(HasHVXD ? "+hvx-double" : "-hvx-double");
+  Features.push_back(UseLongCalls ? "+long-calls" : "-long-calls");
 }
 
 static void getWebAssemblyTargetFeatures(const ArgList &Args,
                                          std::vector<const char *> &Features) {
   handleTargetFeaturesGroup(Args, Features, options::OPT_m_wasm_Features_Group);
 }
 
 static void getAMDGPUTargetFeatures(const Driver &D, const ArgList &Args,

test/Driver/hexagon-long-calls.c

+// RUN: %clang -target hexagon -### %s 2>&1 \
+// RUN:    | FileCheck %s -check-prefix CHECK-DEFAULT
+
+// RUN: %clang -target hexagon -### -mlong-calls %s 2>&1 \
+// RUN:    | FileCheck %s -check-prefix CHECK-LONG-CALLS
+
+// RUN: %clang -target hexagon -### -mlong-calls -mno-long-calls %s 2>&1 \
+// RUN:    | FileCheck %s -check-prefix CHECK-NO-LONG-CALLS
+
+// CHECK-DEFAULT-NOT: "-target-feature" "+long-calls"
+
+// CHECK-LONG-CALLS: "-target-feature" "+long-calls"
+
+// CHECK-NO-LONG-CALLS-NOT: "-target-feature" "+long-calls"
+