[clang] Implement -falign-loops=N (N is a power of 2) for non-LTO

[MaskRay]

Summary

GCC supports multiple forms of -falign-loops=.
-falign-loops= is currently ignored in Clang.

This patch implements the simplest but the most useful form where N is a
power of 2.

The underlying implementation uses a llvm::TargetOptions option for now.
Bitcode generation ignores this option.
The user can specify a global -Wl,-plugin-opt=-align-loops=128.

[efriedma]

Can we hook this up to a LLVM IR function attribute, instead of making it a codegen flag?

[MaskRay]

In D106701#2901639, @efriedma wrote:

Can we hook this up to a LLVM IR function attribute, instead of making it a codegen flag?

The current TargetLoweringBase::PrefLoopAlignment is global. I have considered a function attribute, but it seems overkill for now.
(Inlining behavior is a bit unclear.)
The current use cases just need a global value instead of a refined per-function value.

[luismarques]

LGTM. I'll let someone familiar with the old option explicitly approve it.

clang/test/Driver/falign-loops.c

7–8

I would generally expect to see the <= x bound tested with x and x+1, not just x+1.

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

Nit: it's a convention of the RISC-V backend codegen tests to wrap the RUN lines.

[MaskRay]

comments. add a test/CodeGen test. add HelpText.

[dblaikie]

In D106701#2901656, @MaskRay wrote:

In D106701#2901639, @efriedma wrote:

Can we hook this up to a LLVM IR function attribute, instead of making it a codegen flag?

The current TargetLoweringBase::PrefLoopAlignment is global. I have considered a function attribute, but it seems overkill for now.
(Inlining behavior is a bit unclear.)
The current use cases just need a global value instead of a refined per-function value.

global module metadata is also an option

(what's the motivation for adding this feature - do you have a use-case in mind?)

(the usual: This should probably be committed as separate patches - at least LLVM, then Clang pieces)

[MaskRay]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

only 86 columns. compiler-rt is even transiting to 100 column.

[craig.topper]

In D106701#2902544, @dblaikie wrote:

In D106701#2901656, @MaskRay wrote:

In D106701#2901639, @efriedma wrote:

Can we hook this up to a LLVM IR function attribute, instead of making it a codegen flag?

The current TargetLoweringBase::PrefLoopAlignment is global. I have considered a function attribute, but it seems overkill for now.
(Inlining behavior is a bit unclear.)
The current use cases just need a global value instead of a refined per-function value.

global module metadata is also an option

(what's the motivation for adding this feature - do you have a use-case in mind?)

(the usual: This should probably be committed as separate patches - at least LLVM, then Clang pieces)

I would like to have this for experimenting on RISCV. I was proposing to add similar hidden options like X86 in D106570.

[jrtc27]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

compiler-rt is not the RISC-V backend :)

10 ↗

(On Diff #361303)

This isn't autogenerated? Also NOT on .p2align isn't great in general, .balign and .align don't match that yet could have been emitted.

13 ↗

(On Diff #361303)

• not _ in check prefixes

[MaskRay]

In D106701#2902544, @dblaikie wrote:

In D106701#2901656, @MaskRay wrote:

In D106701#2901639, @efriedma wrote:

Can we hook this up to a LLVM IR function attribute, instead of making it a codegen flag?

The current TargetLoweringBase::PrefLoopAlignment is global. I have considered a function attribute, but it seems overkill for now.
(Inlining behavior is a bit unclear.)
The current use cases just need a global value instead of a refined per-function value.

global module metadata is also an option

Using a global module metadata needs to think of the merging behavior.
The behavior isn't clear.

(what's the motivation for adding this feature - do you have a use-case in mind?)

Use case: x86 has a cl::opt. RISC-V is exploring D106570.

(the usual: This should probably be committed as separate patches - at least LLVM, then Clang pieces)

(Can commit the llvm/ part first.)

[MaskRay]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

Wrapping lines here just makes the code less readable.

13 ↗

(On Diff #361303)

I find that _ in check prefixes is also popular.

It has the benefit that _ cannot conflict with -NOT -LABEL` etc.

[jrtc27]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

That's your personal opinion, which I disagree with, and it's not true if your terminal isn't wide enough. Going against existing convention in the backend tests should only be done with very good reason, and personal opinion is not that.

13 ↗

(On Diff #361303)

I have never seen it before and there are zero uses of it in RISC-V CodeGen tests. Please conform to the existing style by using -.

[MaskRay]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

Lines longer than 80-column (in this case just 86) are pretty common among tests. I really hope test/CodeGen/RISCV/ can be more tolerant on this matter.

Even the Linux scripts/checkpatch.pl has increased the limit to 100 because in many cases wrapping lines for strict 80-conformance just harms readability.

Of course I don't want to waste time arguing on this matter. So if this turns out to be an issue for RISC-V folks, I'll update it to save my time.

[luismarques]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

Of course I don't want to waste time arguing on this matter. So if this turns out to be an issue for RISC-V folks, I'll update it to save my time.

Personally, I don't particularly care. I don't know if @asb has strong feelings about this. If you think it would be beneficial to relax this convention please raise the issue on llvm-dev. Let's not keep discussing this in every patch touching RISC-V :-)

[MaskRay]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

Personally I don't even think the generic case needs to be raised on llvm-dev:) There are just so many column>80 cases in llvm/test and clang/test. Actually, If someone wants to enforce the 80-column rule more rigidly, that probably needs a discussion.

That said, the argument here is about a subdirectory: llvm/test/CodeGen/RISCV/ ...

[asb]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

I don't have a strong view on this one to be honest. I think I've typically wrapped at 80 columns for these RUN lines after being asked to, but ultimately I think choosing a logical point to split has a greater impact on readability than keeping it strictly to 80 columns.

[jrtc27]

llvm/test/CodeGen/RISCV/loop-alignment.ll

3–4 ↗

(On Diff #361303)

FWIW I care less about argument lists extending beyond 80 columns, but I do think the | is a logical point at which to wrap it if you have a long line and keeps things more readable.

[craig.topper]

Does this work with LTO?

[MaskRay]

In D106701#2908679, @craig.topper wrote:

Does this work with LTO?

-falign-loops= doesn't affect linker code generation options.

-Wl,-mllvm,--align-loops=128 can be used for now.

[MaskRay]

Ping.

• -falign-loops= is currently silently ignored.
• -fliang-loops= has user interest from at least x86 and RISC-V.
• This patch makes the driver option work for the non-LTO case and gives the LTO case -Wl,-mllvm,--align-loops=128 (some other features were done this way. They had a -mllvm before a driver option.)

I think this is good enough.

The LTO case require a function attribute (this doesn't apply to synthesized functions)/module flags metadata which may be overkill.

[luismarques]

Still LGTM.

BTW, I liked that in the old version the help string included "In GCC =0 is the same as =1".

[MaskRay]

rebase

[MaskRay]

In D106701#2924638, @luismarques wrote:

Still LGTM.

BTW, I liked that in the old version the help string included "In GCC =0 is the same as =1".

I find that in GCC =0 is not necessarily =1, but not particular clear about its exact behavior;-)

[craig.topper]

clang/lib/Driver/ToolChains/Clang.cpp

4749

gcc 5.4 is throwing a -Wparentheses warning here. I'm in the middle of something else in my tree or I would just fix it. Maybe isPowerOf2_32 would be more readable anyway?

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

4749

maybe just add parens (Value - 1) ... this is probably a quite common pattern. And the line below has err_drv_alignment_not_power_of_two which is self-explanatory.

clang/include/clang/Basic/CodeGenOptions.def

 
 CODEGENOPT(VerifyModule      , 1, 1) ///< Control whether the module should be run
                                      ///< through the LLVM Verifier.
 
 CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to force stack
                                      ///< realignment.
 CODEGENOPT(UseInitArray      , 1, 0) ///< Control whether to use .init_array or
                                      ///< .ctors.
+VALUE_CODEGENOPT(LoopAlignment     , 32, 0) ///< Overrides default loop
+                                            ///< alignment, if not 0.
 VALUE_CODEGENOPT(StackAlignment    , 32, 0) ///< Overrides default stack
                                             ///< alignment, if not 0.
 VALUE_CODEGENOPT(StackProbeSize    , 32, 4096) ///< Overrides default stack
                                                ///< probe size, even if 0.
 VALUE_CODEGENOPT(WarnStackSize     , 32, UINT_MAX) ///< Set via -fwarn-stack-size.
 CODEGENOPT(NoStackArgProbe, 1, 0) ///< Set when -mno-stack-arg-probe is used
 CODEGENOPT(DebugStrictDwarf, 1, 1) ///< Whether or not to use strict DWARF info.
 CODEGENOPT(DebugColumnInfo, 1, 0) ///< Whether or not to use column information

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def warn_drv_yc_multiple_inputs_clang_cl : Warning<
   "support for '/Yc' with more than one source file not implemented yet; flag ignored">,
   InGroup<ClangClPch>;
 
 def err_drv_invalid_value : Error<"invalid value '%1' in '%0'">;
 def err_drv_invalid_int_value : Error<"invalid integral value '%1' in '%0'">;
 def err_drv_invalid_value_with_suggestion : Error<
     "invalid value '%1' in '%0', expected one of: %2">;
+def err_drv_alignment_not_power_of_two : Error<"alignment is not a power of 2 in '%0'">;
 def err_drv_invalid_remap_file : Error<
     "invalid option '%0' not of the form <from-file>;<to-file>">;
 def err_drv_invalid_gcc_output_type : Error<
     "invalid output type '%0' for use with gcc tool">;
 def err_drv_cc_print_options_failure : Error<
     "unable to open CC_PRINT_OPTIONS file: %0">;
 def err_drv_lto_without_lld : Error<"LTO requires -fuse-ld=lld">;
 def err_drv_preamble_format : Error<

clang/include/clang/Basic/LangOptions.def

 BENIGN_LANGOPT(AllowEditorPlaceholders, 1, 0,
                "allow editor placeholders in source")
 
 ENUM_LANGOPT(ClangABICompat, ClangABI, 4, ClangABI::Latest,
              "version of Clang that we should attempt to be ABI-compatible "
              "with")
 
 COMPATIBLE_VALUE_LANGOPT(FunctionAlignment, 5, 0, "Default alignment for functions")
+COMPATIBLE_VALUE_LANGOPT(LoopAlignment, 32, 0, "Default alignment for loops")
 
 LANGOPT(FixedPoint, 1, 0, "fixed point types")
 LANGOPT(PaddingOnUnsignedFixedPoint, 1, 0,
         "unsigned fixed point types having one extra padding bit")
 
 LANGOPT(RegisterStaticDestructors, 1, 1, "Register C++ static destructors")
 
 LANGOPT(MatrixTypes, 1, 0, "Enable or disable the builtin matrix type")

clang/include/clang/Driver/Options.td

 def fPIE : Flag<["-"], "fPIE">, Group<f_Group>;
 def fno_PIE : Flag<["-"], "fno-PIE">, Group<f_Group>;
 defm access_control : BoolFOption<"access-control",
   LangOpts<"AccessControl">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option], "Disable C++ access control">,
   PosFlag<SetTrue>>;
 def falign_functions : Flag<["-"], "falign-functions">, Group<f_Group>;
 def falign_functions_EQ : Joined<["-"], "falign-functions=">, Group<f_Group>;
+def falign_loops_EQ : Joined<["-"], "falign-loops=">, Group<f_Group>, Flags<[CC1Option]>, MetaVarName<"<N>">,
+  HelpText<"N must be a power of two. Align loops to the boundary">,
+  MarshallingInfoInt<CodeGenOpts<"LoopAlignment">>;
 def fno_align_functions: Flag<["-"], "fno-align-functions">, Group<f_Group>;
 defm allow_editor_placeholders : BoolFOption<"allow-editor-placeholders",
   LangOpts<"AllowEditorPlaceholders">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Treat editor placeholders as valid source code">,
   NegFlag<SetFalse>>;
 def fallow_unsupported : Flag<["-"], "fallow-unsupported">, Group<f_Group>;
 def fapple_kext : Flag<["-"], "fapple-kext">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use Apple's kernel extensions ABI">,
def fbinutils_version_EQ : Joined<["-"], "fbinutils-version=">,
   "generated assembly will consider GNU as support. 'none' means that all ELF "
   "features can be used, regardless of binutils support. Defaults to 2.26.">;
 def fuse_ld_EQ : Joined<["-"], "fuse-ld=">, Group<f_Group>, Flags<[CoreOption, LinkOption]>;
 def ld_path_EQ : Joined<["--"], "ld-path=">, Group<Link_Group>;
 
 defm align_labels : BooleanFFlag<"align-labels">, Group<clang_ignored_gcc_optimization_f_Group>;
 def falign_labels_EQ : Joined<["-"], "falign-labels=">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm align_loops : BooleanFFlag<"align-loops">, Group<clang_ignored_gcc_optimization_f_Group>;
-def falign_loops_EQ : Joined<["-"], "falign-loops=">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm align_jumps : BooleanFFlag<"align-jumps">, Group<clang_ignored_gcc_optimization_f_Group>;
 def falign_jumps_EQ : Joined<["-"], "falign-jumps=">, Group<clang_ignored_gcc_optimization_f_Group>;
 
 // FIXME: This option should be supported and wired up to our diognostics, but
 // ignore it for now to avoid breaking builds that use it.
 def fdiagnostics_show_location_EQ : Joined<["-"], "fdiagnostics-show-location=">, Group<clang_ignored_f_Group>;
 
 defm fcheck_new : BooleanFFlag<"check-new">, Group<clang_ignored_f_Group>;

clang/lib/CodeGen/BackendUtil.cpp

static bool initTargetOptions(DiagnosticsEngine &Diags,
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
   Options.ForceDwarfFrameSection = CodeGenOpts.ForceDwarfFrameSection;
   Options.EmitCallSiteInfo = CodeGenOpts.EmitCallSiteInfo;
   Options.EnableAIXExtendedAltivecABI = CodeGenOpts.EnableAIXExtendedAltivecABI;
   Options.PseudoProbeForProfiling = CodeGenOpts.PseudoProbeForProfiling;
   Options.ValueTrackingVariableLocations =
       CodeGenOpts.ValueTrackingVariableLocations;
   Options.XRayOmitFunctionIndex = CodeGenOpts.XRayOmitFunctionIndex;
+  Options.LoopAlignment = CodeGenOpts.LoopAlignment;
 
   Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfFile;
   Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
   Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
   Options.MCOptions.MCUseDwarfDirectory = !CodeGenOpts.NoDwarfDirectoryAsm;
   Options.MCOptions.MCNoExecStack = CodeGenOpts.NoExecStack;
   Options.MCOptions.MCIncrementalLinkerCompatible =
       CodeGenOpts.IncrementalLinkerCompatible;

clang/lib/Driver/ToolChains/Clang.cpp

#endif
 
   unsigned FunctionAlignment = ParseFunctionAlignment(TC, Args);
   assert(FunctionAlignment <= 31 && "function alignment will be truncated!");
   if (FunctionAlignment) {
     CmdArgs.push_back("-function-alignment");
     CmdArgs.push_back(Args.MakeArgString(std::to_string(FunctionAlignment)));
   }
 
+  // We support -falign-loops=N where N is a power of 2. GCC supports more
+  // forms.
+  if (const Arg *A = Args.getLastArg(options::OPT_falign_loops_EQ)) {
+    unsigned Value = 0;
+    if (StringRef(A->getValue()).getAsInteger(10, Value) || Value > 65536)
+      TC.getDriver().Diag(diag::err_drv_invalid_int_value)
+          << A->getAsString(Args) << A->getValue();
+    else if (Value & Value - 1)
+      TC.getDriver().Diag(diag::err_drv_alignment_not_power_of_two)
+          << A->getAsString(Args) << A->getValue();
+    // Treat =0 as unspecified (use the target preference).
+    if (Value)
+      CmdArgs.push_back(Args.MakeArgString("-falign-loops=" +
+                                           Twine(std::min(Value, 65536u))));
+  }
+
   llvm::Reloc::Model RelocationModel;
   unsigned PICLevel;
   bool IsPIE;
   std::tie(RelocationModel, PICLevel, IsPIE) = ParsePICArgs(TC, Args);
 
   bool IsROPI = RelocationModel == llvm::Reloc::ROPI ||
                 RelocationModel == llvm::Reloc::ROPI_RWPI;
   bool IsRWPI = RelocationModel == llvm::Reloc::RWPI ||

clang/test/CodeGen/align-loops.c

+// REQUIRES: x86-registered-target
+/// Check asm because we use llvm::TargetOptions.
+
+// RUN: %clang_cc1 -triple=x86_64 -S %s -falign-loops=8 -O -o - | FileCheck %s --check-prefixes=CHECK,CHECK_8
+// RUN: %clang_cc1 -triple=x86_64 -S %s -falign-loops=32 -O -o - | FileCheck %s --check-prefixes=CHECK,CHECK_32
+
+// CHECK-LABEL: foo:
+// CHECK_8:       .p2align 3, 0x90
+// CHECK_32:      .p2align 5, 0x90
+
+void bar();
+void foo() {
+  for (int i = 0; i < 64; ++i)
+    bar();
+}

clang/test/Driver/clang_f_opts.c

 // CHECK-WARNING-DAG: optimization flag '-fstrength-reduce' is not supported
 // CHECK-WARNING-DAG: optimization flag '-ftracer' is not supported
 // CHECK-WARNING-DAG: optimization flag '-funroll-all-loops' is not supported
 // CHECK-WARNING-DAG: optimization flag '-funswitch-loops' is not supported
 // CHECK-WARNING-DAG: unsupported argument '1' to option 'flto='
 // CHECK-WARNING-DAG: optimization flag '-falign-labels' is not supported
 // CHECK-WARNING-DAG: optimization flag '-falign-labels=100' is not supported
 // CHECK-WARNING-DAG: optimization flag '-falign-loops' is not supported
-// CHECK-WARNING-DAG: optimization flag '-falign-loops=100' is not supported
 // CHECK-WARNING-DAG: optimization flag '-falign-jumps' is not supported
 // CHECK-WARNING-DAG: optimization flag '-falign-jumps=100' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fexcess-precision=100' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fbranch-count-reg' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fcaller-saves' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fno-default-inline' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fgcse-after-reload' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fgcse-las' is not supported

clang/test/Driver/falign-loops.c

+/// Treat -falign-loops=0 as not specifying the option.
+// RUN: %clang -### -falign-loops=0 %s 2>&1 | FileCheck %s --check-prefix=CHECK_NO
+// RUN: %clang -### -falign-loops=1 %s 2>&1 | FileCheck %s --check-prefix=CHECK_1
+// RUN: %clang -### -falign-loops=4 %s 2>&1 | FileCheck %s --check-prefix=CHECK_4
+/// Only powers of 2 are supported for now.
+// RUN: %clang -### -falign-loops=5 %s 2>&1 | FileCheck %s --check-prefix=CHECK_5
+// RUN: %clang -### -falign-loops=65536 %s 2>&1 | FileCheck %s --check-prefix=CHECK_65536
+// RUN: %clang -### -falign-loops=65537 %s 2>&1 | FileCheck %s --check-prefix=CHECK_65537
+// RUN: %clang -### -falign-loops=a %s 2>&1 | FileCheck %s --check-prefix=CHECK_ERR_A
+
+// CHECK_NO-NOT: "-falign-loops=
+// CHECK_1: "-falign-loops=1"
+// CHECK_4: "-falign-loops=4"
+// CHECK_5: error: alignment is not a power of 2 in '-falign-loops=5'
+// CHECK_65536: "-falign-loops=65536"
+// CHECK_65537: error: invalid integral value '65537' in '-falign-loops=65537'
+// CHECK_ERR_A: error: invalid integral value 'a' in '-falign-loops=a'