[clang][PassManager] Add -falways-mem2reg CC1 flag to run mem2reg at -O0

[jrtc27]

Summary

Standard -O0 IR and assembly can be hard to follow as, without mem2reg,
there are loads and stores to the stack everywhere that clutter things
up and make it hard to see where the actual interesting instructions are
(such as when trying to debug a crash in unoptimised code from the
disassembly). It is therefore useful to be able to force mem2reg to be
run even at -O0 to clean up a lot of those stack loads and stores. There
are also Clang CodeGen tests in the tree that explicitly run mem2reg on
the output in order to make the CHECK lines more readable, which
requires manually passing -disable-O0-optnone and piping to opt; having
a flag that supports this also makes those less clunky.

Whilst optimisation for speed's sake is not the primary purpose of this
patch, it does provide an easy significant improvement in code size as
you might expect, giving a ~12% decrease in code size on macOS/arm64
when compiling Clang itself with the option enabled, likely also having
a significant improvement on the running time of the test suite over a
plain Debug build. On GNU/Linux/amd64 the decrease is less pronounced,
at about 4%, likely due to the fact that many instructions can take one
memory operand and so do not have to pay the additional cost of a load
or store like on load-store architectures.

[jrtc27]

Ping

[efriedma]

I think it would be better to focus on making -O1 more usable for this sort of purpose, rather than introduce -O0.5. I mean, there's a lot of wiggle-room between -O0 and -O2, but I don't think it makes sense to add a driver option that promises to run exactly one optimization.

[jrtc27]

This is not meant to be an -O0.5, this is meant to be an -Oepsilon. I don't want optimised code, I just want code that I can actually disassemble and understand without having to trawl through a mess of stack spills and loads. This is for debugging really basic bugs (either compiler or bad C/C++ input) that turn up even at -O0 and that you don't want optimisations for because it's just going to complicate things, or even make the bugs disappear.

This is also so that the myriad of %clang_cc1 -disable-O0-optnone | opt -S -mem2reg seen in clang/tests can become %clang_cc1 -falways-mem2reg as the current way to write those tests is really clunky.

[efriedma]

The part I'm most uncomfortable with is sticking "mem2reg" in a public, documented driver option. I don't want to promise that the mem2reg pass will exist forever. We should be focused on making sure the options we add are stable, and compose effectively, not just being convenient for some specific use.

I'd be less concerned if it were just a -cc1 option; if it's for our internal use, and we can throw it away if we come up with a better solution, this seems okay.

[jrtc27]

In D105516#2889411, @efriedma wrote:

The part I'm most uncomfortable with is sticking "mem2reg" in a public, documented driver option. I don't want to promise that the mem2reg pass will exist forever. We should be focused on making sure the options we add are stable, and compose effectively, not just being convenient for some specific use.

I'd be less concerned if it were just a -cc1 option; if it's for our internal use, and we can throw it away if we come up with a better solution, this seems okay.

I'd be ok with having it just be a -cc1 option (I didn't even actually add a driver test for the non-cc1 form...). I also thought about doing something like -falways-regalloc to not tie it to the pass name, but names like that are misleading since machine register allocation does still happen, just not on things that it doesn't know could be promoted from memory to registers.

[rjmccall]

I agree with Eli: we should decide what the goals are here and then use those goals to decide if we can identify a desirable permanent feature and, if so, what the appropriate name for that feature is.

It sounds like your goal is to get readable assembly that still corresponds fairly literally to your original code, in the sense that the readability of -O0 assembly is often undermined by the sheer amount of code and all the extra, unnecessary work it seems to do. However, I've found that a lot of the extra -O0 code is not actually from loads and stores to local variables, it's from the -O0 instruction selection and register allocation, which often combine to do very silly things. Have you looked into whether you get more readable code by just running normal -O0 IR through the non-O0 codegen pipeline? Because the problem with doing just mem2reg is that that's already a fairly major non-literal change to the code, and at some point it's tricky to say what exactly should be part of this new pipeline; whereas still emitting exactly what the abstract machine says to do, just with less nonsense from fast-isel, is a lot easier to define.

[jrtc27]

Now only a CC1 option

[efriedma]

Seems fine.

[arichardson]

I like this since the flag significantly improves readability for update_cc_test_checks.py-generated Clang test without having to use the -disable-O0-optnone | opt trick. Not sure what the best flag name is, but as long as it's a CC1 flag it shouldn't really matter.

[jrtc27]

In D105516#2892512, @rjmccall wrote:

I agree with Eli: we should decide what the goals are here and then use those goals to decide if we can identify a desirable permanent feature and, if so, what the appropriate name for that feature is.

It sounds like your goal is to get readable assembly that still corresponds fairly literally to your original code, in the sense that the readability of -O0 assembly is often undermined by the sheer amount of code and all the extra, unnecessary work it seems to do. However, I've found that a lot of the extra -O0 code is not actually from loads and stores to local variables, it's from the -O0 instruction selection and register allocation, which often combine to do very silly things. Have you looked into whether you get more readable code by just running normal -O0 IR through the non-O0 codegen pipeline? Because the problem with doing just mem2reg is that that's already a fairly major non-literal change to the code, and at some point it's tricky to say what exactly should be part of this new pipeline; whereas still emitting exactly what the abstract machine says to do, just with less nonsense from fast-isel, is a lot easier to define.

Well, I'm dealing with RISC-V which doesn't have FastISel. And no, running -O0 IR through a different pipeline is never going to work well without mem2reg (and you can't mem2reg -O0 IR without -disable-O0-optnone), so you need at least this to work well. Whether or not there's a useful load of additional stuff you could do, maybe, but I do think this patch in and of itself makes a huge difference, and the limited scope of it can be beneficial.

[jrtc27]

@rjmccall Ping?

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(TimePasses        , 1, 0) ///< Set when -ftime-report or -ftime-report= is enabled.
 CODEGENOPT(TimePassesPerRun  , 1, 0) ///< Set when -ftime-report=per-pass-run is enabled.
 CODEGENOPT(TimeTrace         , 1, 0) ///< Set when -ftime-trace is enabled.
 VALUE_CODEGENOPT(TimeTraceGranularity, 32, 500) ///< Minimum time granularity (in microseconds),
                                                ///< traced by time profiler
 CODEGENOPT(UnrollLoops       , 1, 0) ///< Control whether loops are unrolled.
 CODEGENOPT(RerollLoops       , 1, 0) ///< Control whether loops are rerolled.
 CODEGENOPT(NoUseJumpTables   , 1, 0) ///< Set when -fno-jump-tables is enabled.
+CODEGENOPT(AlwaysMem2Reg     , 1, 0) ///< Set when -falways-mem2reg is enabled.
 CODEGENOPT(UnwindTables      , 1, 0) ///< Emit unwind tables.
 CODEGENOPT(VectorizeLoop     , 1, 0) ///< Run loop vectorizer.
 CODEGENOPT(VectorizeSLP      , 1, 0) ///< Run SLP vectorizer.
 CODEGENOPT(ProfileSampleAccurate, 1, 0) ///< Sample profile is accurate.
 
 /// Treat loops as finite: language, always, never.
 ENUM_CODEGENOPT(FiniteLoops, FiniteLoopsKind, 2, FiniteLoopsKind::Language)
 

clang/include/clang/Driver/Options.td

def fno_unroll_loops : Flag<["-"], "fno-unroll-loops">, Group<f_Group>,
   HelpText<"Turn off loop unroller">, Flags<[CC1Option]>;
 defm reroll_loops : BoolFOption<"reroll-loops",
   CodeGenOpts<"RerollLoops">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Turn on loop reroller">, NegFlag<SetFalse>>;
 def ffinite_loops: Flag<["-"],  "ffinite-loops">, Group<f_Group>,
   HelpText<"Assume all loops are finite.">, Flags<[CC1Option]>;
 def fno_finite_loops: Flag<["-"], "fno-finite-loops">, Group<f_Group>,
   HelpText<"Do not assume that any loop is finite.">, Flags<[CC1Option]>;
+defm always_mem2reg : BoolFOption<"always-mem2reg",
+  CodeGenOpts<"AlwaysMem2Reg">, DefaultFalse,
+  PosFlag<SetTrue, [], "Always run mem2reg regardless of optimisation level">,
+  NegFlag<SetFalse, [], "Run mem2reg based on optimisation level">,
+  BothFlags<[CC1Option]>>;
 
 def ftrigraphs : Flag<["-"], "ftrigraphs">, Group<f_Group>,
   HelpText<"Process trigraph sequences">, Flags<[CC1Option]>;
 def fno_trigraphs : Flag<["-"], "fno-trigraphs">, Group<f_Group>,
   HelpText<"Do not process trigraph sequences">, Flags<[CC1Option]>;
 def funsigned_bitfields : Flag<["-"], "funsigned-bitfields">, Group<f_Group>;
 def funsigned_char : Flag<["-"], "funsigned-char">, Group<f_Group>;
 def fno_unsigned_char : Flag<["-"], "fno-unsigned-char">;

clang/lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::CreatePasses(legacy::PassManager &MPM,
   PMBuilder.SizeLevel = CodeGenOpts.OptimizeSize;
   PMBuilder.SLPVectorize = CodeGenOpts.VectorizeSLP;
   PMBuilder.LoopVectorize = CodeGenOpts.VectorizeLoop;
   // Only enable CGProfilePass when using integrated assembler, since
   // non-integrated assemblers don't recognize .cgprofile section.
   PMBuilder.CallGraphProfile = !CodeGenOpts.DisableIntegratedAS;
 
   PMBuilder.DisableUnrollLoops = !CodeGenOpts.UnrollLoops;
+  PMBuilder.AlwaysMem2Reg = CodeGenOpts.AlwaysMem2Reg;
   // Loop interleaving in the loop vectorizer has historically been set to be
   // enabled when loop unrolling is enabled.
   PMBuilder.LoopsInterleaved = CodeGenOpts.UnrollLoops;
   PMBuilder.MergeFunctions = CodeGenOpts.MergeFunctions;
   PMBuilder.PrepareForThinLTO = CodeGenOpts.PrepareForThinLTO;
   PMBuilder.PrepareForLTO = CodeGenOpts.PrepareForLTO;
   PMBuilder.RerollLoops = CodeGenOpts.RerollLoops;
 
void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
 
   PipelineTuningOptions PTO;
   PTO.LoopUnrolling = CodeGenOpts.UnrollLoops;
   // For historical reasons, loop interleaving is set to mirror setting for loop
   // unrolling.
   PTO.LoopInterleaving = CodeGenOpts.UnrollLoops;
   PTO.LoopVectorization = CodeGenOpts.VectorizeLoop;
   PTO.SLPVectorization = CodeGenOpts.VectorizeSLP;
+  PTO.AlwaysMem2Reg = CodeGenOpts.AlwaysMem2Reg;
   PTO.MergeFunctions = CodeGenOpts.MergeFunctions;
   // Only enable CGProfilePass when using integrated assembler, since
   // non-integrated assemblers don't recognize .cgprofile section.
   PTO.CallGraphProfile = !CodeGenOpts.DisableIntegratedAS;
   PTO.Coroutines = LangOpts.Coroutines;
 
   LoopAnalysisManager LAM;
   FunctionAnalysisManager FAM;

clang/lib/CodeGen/CodeGenModule.cpp

if (!D) {
     F->addAttributes(llvm::AttributeList::FunctionIndex, B);
     return;
   }
 
   // Track whether we need to add the optnone LLVM attribute,
   // starting with the default for this optimization level.
   bool ShouldAddOptNone =
       !CodeGenOpts.DisableO0ImplyOptNone && CodeGenOpts.OptimizationLevel == 0;
+  // -falways-mem2reg implies at least a minimal amount of optimisation.
+  ShouldAddOptNone &= !CodeGenOpts.AlwaysMem2Reg;
   // We can't add optnone in the following cases, it won't pass the verifier.
   ShouldAddOptNone &= !D->hasAttr<MinSizeAttr>();
   ShouldAddOptNone &= !D->hasAttr<AlwaysInlineAttr>();
 
   // Add optnone, but do so only if the function isn't always_inline.
   if ((ShouldAddOptNone || D->hasAttr<OptimizeNoneAttr>()) &&
       !F->hasFnAttribute(llvm::Attribute::AlwaysInline)) {
     B.addAttribute(llvm::Attribute::OptimizeNone);

clang/lib/Driver/ToolChains/Clang.cpp

if (A->getOption().matches(options::OPT_freroll_loops))
       CmdArgs.push_back("-freroll-loops");
 
   Args.AddLastArg(CmdArgs, options::OPT_ffinite_loops,
                   options::OPT_fno_finite_loops);
 
   Args.AddLastArg(CmdArgs, options::OPT_fwritable_strings);
   Args.AddLastArg(CmdArgs, options::OPT_funroll_loops,
                   options::OPT_fno_unroll_loops);
+  Args.AddLastArg(CmdArgs, options::OPT_falways_mem2reg,
+                  options::OPT_fno_always_mem2reg);
 
   Args.AddLastArg(CmdArgs, options::OPT_pthread);
 
   if (Args.hasFlag(options::OPT_mspeculative_load_hardening,
                    options::OPT_mno_speculative_load_hardening, false))
     CmdArgs.push_back(Args.MakeArgString("-mspeculative-load-hardening"));
 
   RenderSSPOptions(D, TC, Args, CmdArgs, KernelOrKext);

clang/test/CodeGen/falways-mem2reg.c

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -flegacy-pass-manager -O0 %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -fno-legacy-pass-manager -O0 %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -flegacy-pass-manager -O0 -fno-always-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -fno-legacy-pass-manager -O0 -fno-always-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -flegacy-pass-manager -O0 -falways-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-MEM2REG
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -fno-legacy-pass-manager -O0 -falways-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-MEM2REG
+
+// O0-NO-MEM2REG-LABEL: @add(
+// O0-NO-MEM2REG-NEXT:  entry:
+// O0-NO-MEM2REG-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
+// O0-NO-MEM2REG-NEXT:    [[B_ADDR:%.*]] = alloca i32, align 4
+// O0-NO-MEM2REG-NEXT:    store i32 [[A:%.*]], i32* [[A_ADDR]], align 4
+// O0-NO-MEM2REG-NEXT:    store i32 [[B:%.*]], i32* [[B_ADDR]], align 4
+// O0-NO-MEM2REG-NEXT:    [[TMP0:%.*]] = load i32, i32* [[A_ADDR]], align 4
+// O0-NO-MEM2REG-NEXT:    [[TMP1:%.*]] = load i32, i32* [[B_ADDR]], align 4
+// O0-NO-MEM2REG-NEXT:    [[ADD:%.*]] = add nsw i32 [[TMP0]], [[TMP1]]
+// O0-NO-MEM2REG-NEXT:    ret i32 [[ADD]]
+//
+// O0-MEM2REG-LABEL: @add(
+// O0-MEM2REG-NEXT:  entry:
+// O0-MEM2REG-NEXT:    [[ADD:%.*]] = add nsw i32 [[A:%.*]], [[B:%.*]]
+// O0-MEM2REG-NEXT:    ret i32 [[ADD]]
+//
+int add(int a, int b) {
+  return a + b;
+}

llvm/include/llvm/Passes/PassBuilder.h

public:
 
   /// Tuning option to enable/disable loop unrolling. Its default value is true.
   bool LoopUnrolling;
 
   /// Tuning option to forget all SCEV loops in LoopUnroll. Its default value
   /// is that of the flag: `-forget-scev-loop-unroll`.
   bool ForgetAllSCEVInLoopUnroll;
 
+  /// Tuning option to always run mem2reg regardless of the optimisation level.
+  /// Its default value is false.
+  bool AlwaysMem2Reg;
+
   /// Tuning option to enable/disable coroutine intrinsic lowering. Its default
   /// value is false. Frontends such as Clang may enable this conditionally. For
   /// example, Clang enables this option if the flags `-std=c++2a` or above, or
   /// `-fcoroutines-ts`, have been specified.
   bool Coroutines;
 
   /// Tuning option to cap the number of calls to retrive clobbering accesses in
   /// MemorySSA, in LICM.

llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h

public:
 
   /// The module summary index to use for importing information to the
   /// thin LTO backends, for example for the CFI and devirtualization type
   /// tests.
   const ModuleSummaryIndex *ImportSummary = nullptr;
 
   bool DisableTailCalls;
   bool DisableUnrollLoops;
+  bool AlwaysMem2Reg;
   bool CallGraphProfile;
   bool SLPVectorize;
   bool LoopVectorize;
   bool LoopsInterleaved;
   bool RerollLoops;
   bool NewGVN;
   bool DisableGVNLoadPRE;
   bool ForgetAllSCEVInLoopUnroll;

llvm/lib/Passes/PassBuilder.cpp

static cl::opt<bool> EnableO3NonTrivialUnswitching(
     cl::ZeroOrMore, cl::desc("Enable non-trivial loop unswitching for -O3"));
 
 PipelineTuningOptions::PipelineTuningOptions() {
   LoopInterleaving = true;
   LoopVectorization = true;
   SLPVectorization = false;
   LoopUnrolling = true;
   ForgetAllSCEVInLoopUnroll = ForgetSCEVInLoopUnroll;
+  AlwaysMem2Reg = false;
   Coroutines = false;
   LicmMssaOptCap = SetLicmMssaOptCap;
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
   CallGraphProfile = true;
   MergeFunctions = false;
 }
 
 namespace llvm {
ModulePassManager PassBuilder::buildO0DefaultPipeline(OptimizationLevel Level,
   // which is just that always inlining occurs. Further, disable generating
   // lifetime intrinsics to avoid enabling further optimizations during
   // code generation.
   // However, we need to insert lifetime intrinsics to avoid invalid access
   // caused by multithreaded coroutines.
   MPM.addPass(AlwaysInlinerPass(
       /*InsertLifetimeIntrinsics=*/PTO.Coroutines));
 
+  if (PTO.AlwaysMem2Reg)
+    MPM.addPass(createModuleToFunctionPassAdaptor(PromotePass()));
+
   if (PTO.MergeFunctions)
     MPM.addPass(MergeFunctionsPass());
 
   if (EnableMatrix)
     MPM.addPass(
         createModuleToFunctionPassAdaptor(LowerMatrixIntrinsicsPass(true)));
 
   if (!CGSCCOptimizerLateEPCallbacks.empty()) {

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

 } // namespace llvm
 
 PassManagerBuilder::PassManagerBuilder() {
     OptLevel = 2;
     SizeLevel = 0;
     LibraryInfo = nullptr;
     Inliner = nullptr;
     DisableUnrollLoops = false;
+    AlwaysMem2Reg = false;
     SLPVectorize = false;
     LoopVectorize = true;
     LoopsInterleaved = true;
     RerollLoops = RunLoopRerolling;
     NewGVN = RunNewGVN;
     LicmMssaOptCap = SetLicmMssaOptCap;
     LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
     DisableGVNLoadPRE = false;
if (!PGOSampleUse.empty()) {
     if (!(FlattenedProfileUsed && PerformThinLTO))
       MPM.add(createSampleProfileLoaderPass(PGOSampleUse));
   }
 
   // Allow forcing function attributes as a debugging and tuning aid.
   MPM.add(createForceFunctionAttrsLegacyPass());
 
   // If all optimizations are disabled, just run the always-inline pass and,
-  // if enabled, the function merging pass.
+  // if enabled, the mem2reg and function merging passes.
   if (OptLevel == 0) {
+    if (AlwaysMem2Reg)
+      MPM.add(createPromoteMemoryToRegisterPass());
+
     addPGOInstrPasses(MPM);
     if (Inliner) {
       MPM.add(Inliner);
       Inliner = nullptr;
     }
 
     // FIXME: The BarrierNoopPass is a HACK! The inliner pass above implicitly
     // creates a CGSCC pass manager, but we don't want to add extensions into