r187969 - The only useful loop unrolling flag to give realistically is

Hal Finkel hfinkel at anl.gov
Mon Aug 19 20:53:42 PDT 2013


----- Original Message -----
> ----- Original Message -----
> > Author: chandlerc
> > Date: Thu Aug  8 03:34:35 2013
> > New Revision: 187969
> > 
> > URL: http://llvm.org/viewvc/llvm-project?rev=187969&view=rev
> > Log:
> > The only useful loop unrolling flag to give realistically is
> > '-fno-unroll-loops'. The option to the backend is even called
> > 'DisableUnrollLoops'. This is precisely the form that Clang
> > *didn't*
> > support.
> 
> Maybe we should hook up the positive form to enabling the
> more-aggressive unrolling options: -unroll-allow-partial
> -unroll-runtime -- doing these kinds of unrolling is not always a
> win (I think it gives a mean regression on x86_64, but it gives a
> mean speedup on my A2 cores), but I believe that gcc does equivalent
> things when -funroll-loops is given.
> 
> Also, even with this change, -fno-unroll-loops with -O2 or -O3 might
> not really prevent all loop unrolling because the loop vectorizer
> also unrolls loops based on ILP (and register pressure)
> considerations. I think that passing -force-vector-unroll=1 would
> turn this off.

Chandler, thoughts?

 -Hal

> 
>  -Hal
> 
> > We didn't recognize the flag, we didn't pass it to the CC1
> > layer, and even if we did we wouldn't use it. Clang only inspected
> > the
> > positive form of the flag, and only did so to enable loop unrolling
> > when
> > the optimization level wasn't high enough. This only occurs for an
> > optimization level that even has a chance of running the loop
> > unroller
> > when optimizing for size.
> > 
> > This commit wires up the 'no' variant, and switches the code to
> > actually
> > follow the standard flag pattern of using the last flag and
> > allowing
> > a flag in either direction to override the default.
> > 
> > I think this is still wrong. I don't know why we disable the loop
> > unroller entirely *from Clang* when optimizing for size, as the
> > loop
> > unrolling pass *already has special logic* for the case where the
> > function is attributed as optimized for size! We should really be
> > trusting that. Maybe in a follow-up patch, I don't really want to
> > change
> > behavior here.
> > 
> > Modified:
> >     cfe/trunk/include/clang/Driver/Options.td
> >     cfe/trunk/lib/Driver/Tools.cpp
> >     cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> >     cfe/trunk/test/Driver/clang_f_opts.c
> > 
> > Modified: cfe/trunk/include/clang/Driver/Options.td
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=187969&r1=187968&r2=187969&view=diff
> > ==============================================================================
> > --- cfe/trunk/include/clang/Driver/Options.td (original)
> > +++ cfe/trunk/include/clang/Driver/Options.td Thu Aug  8 03:34:35
> > 2013
> > @@ -791,6 +791,8 @@ def ftrap_function_EQ : Joined<["-"], "f
> >  def funit_at_a_time : Flag<["-"], "funit-at-a-time">,
> >  Group<f_Group>;
> >  def funroll_loops : Flag<["-"], "funroll-loops">, Group<f_Group>,
> >    HelpText<"Turn on loop unroller">, Flags<[CC1Option]>;
> > +def fno_unroll_loops : Flag<["-"], "fno-unroll-loops">,
> > Group<f_Group>,
> > +  HelpText<"Turn off loop unroller">, Flags<[CC1Option]>;
> >  def funsigned_bitfields : Flag<["-"], "funsigned-bitfields">,
> >  Group<f_Group>;
> >  def funsigned_char : Flag<["-"], "funsigned-char">,
> >  Group<f_Group>;
> >  def funwind_tables : Flag<["-"], "funwind-tables">,
> >  Group<f_Group>;
> > 
> > Modified: cfe/trunk/lib/Driver/Tools.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=187969&r1=187968&r2=187969&view=diff
> > ==============================================================================
> > --- cfe/trunk/lib/Driver/Tools.cpp (original)
> > +++ cfe/trunk/lib/Driver/Tools.cpp Thu Aug  8 03:34:35 2013
> > @@ -2977,7 +2977,8 @@ void Clang::ConstructJob(Compilation &C,
> >        CmdArgs.push_back("-fwrapv");
> >    }
> >    Args.AddLastArg(CmdArgs, options::OPT_fwritable_strings);
> > -  Args.AddLastArg(CmdArgs, options::OPT_funroll_loops);
> > +  Args.AddLastArg(CmdArgs, options::OPT_funroll_loops,
> > +                  options::OPT_fno_unroll_loops);
> >  
> >    Args.AddLastArg(CmdArgs, options::OPT_pthread);
> >  
> > 
> > Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=187969&r1=187968&r2=187969&view=diff
> > ==============================================================================
> > --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
> > +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Thu Aug  8
> > 03:34:35
> > 2013
> > @@ -351,8 +351,9 @@ static bool ParseCodeGenArgs(CodeGenOpti
> >    Opts.OptimizeSize = getOptimizationLevelSize(Args);
> >    Opts.SimplifyLibCalls = !(Args.hasArg(OPT_fno_builtin) ||
> >                              Args.hasArg(OPT_ffreestanding));
> > -  Opts.UnrollLoops = Args.hasArg(OPT_funroll_loops) ||
> > -                     (Opts.OptimizationLevel > 1 &&
> > !Opts.OptimizeSize);
> > +  Opts.UnrollLoops =
> > +      Args.hasFlag(OPT_funroll_loops, OPT_fno_unroll_loops,
> > +                   (Opts.OptimizationLevel > 1 &&
> > !Opts.OptimizeSize));
> >  
> >    Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
> >    Opts.AsmVerbose = Args.hasArg(OPT_masm_verbose);
> > 
> > Modified: cfe/trunk/test/Driver/clang_f_opts.c
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/clang_f_opts.c?rev=187969&r1=187968&r2=187969&view=diff
> > ==============================================================================
> > --- cfe/trunk/test/Driver/clang_f_opts.c (original)
> > +++ cfe/trunk/test/Driver/clang_f_opts.c Thu Aug  8 03:34:35 2013
> > @@ -37,6 +37,13 @@
> >  // FP-CONTRACT-FAST-CHECK: -ffp-contract=fast
> >  // FP-CONTRACT-OFF-CHECK: -ffp-contract=off
> >  
> > +// RUN: %clang -### -S -funroll-loops %s 2>&1 | FileCheck
> > -check-prefix=CHECK-UNROLL-LOOPS %s
> > +// RUN: %clang -### -S -fno-unroll-loops %s 2>&1 | FileCheck
> > -check-prefix=CHECK-NO-UNROLL-LOOPS %s
> > +// RUN: %clang -### -S -fno-unroll-loops -funroll-loops %s 2>&1 |
> > FileCheck -check-prefix=CHECK-UNROLL-LOOPS %s
> > +// RUN: %clang -### -S -funroll-loops -fno-unroll-loops %s 2>&1 |
> > FileCheck -check-prefix=CHECK-NO-UNROLL-LOOPS %s
> > +// CHECK-UNROLL-LOOPS: "-funroll-loops"
> > +// CHECK-NO-UNROLL-LOOPS: "-fno-unroll-loops"
> > +
> >  // RUN: %clang -### -S -fvectorize %s 2>&1 | FileCheck
> >  -check-prefix=CHECK-VECTORIZE %s
> >  // RUN: %clang -### -S -fno-vectorize -fvectorize %s 2>&1 |
> >  FileCheck -check-prefix=CHECK-VECTORIZE %s
> >  // RUN: %clang -### -S -fno-vectorize %s 2>&1 | FileCheck
> >  -check-prefix=CHECK-NO-VECTORIZE %s
> > 
> > 
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory



More information about the cfe-commits mailing list