[PassManagerBuilder] Add option for interleaved loops, for loop vectorize.

[asbirlea]

Summary

Match NewPassManager behavior: add option for interleaved loops in the
old pass manager, and use that instead of the flag used to disable loop unroll.
No changes in the defaults.

[chandlerc]

How does this impact use of clang -fno-unroll-loops and clang -fno-loop-vectorize? I'm betting today, -fno-unroll-loops controls the DisableUnrollLoops value, and so people who disable both unrolling and vectorization will be surprised when interleaving now happens.

I think we want to set this to true in clang only when both unrolling and vectorization are enabled as a coarse approximation if the separate flag here, mostly because I think those most impacted will be those who disable one or the other and don't want this behavior in that case.

[asbirlea]

Is it correct to say that coupling this change with a clang change in:

lib/CodeGen/BackendUtil.cpp:546
PMBuilder.DisableUnrollLoops = !CodeGenOpts.UnrollLoops;

To add:

PMBuilder. LoopsInterleaved = CodeGenOpts.UnrollLoops;

AFAICT this is the only place this is set in clang.
Then defer when/if the separation is to ever happen to clang itself?

[asbirlea]

Update test.

[hsaito]

In D61030#1478957, @asbirlea wrote:

Update test.

Have you already had an RFC regarding this change? If so, please add a link here. I bet there are a reasonable number of people who are dependent on vectorizer's interleave behavior is hooked up to the unroll flag. I support the direction as being the right thing to do, but we better give a good warning signal so that the affected people will scope the work. Do we already have a clang level interleave control flag? Else, those who are using unroll flag to control interleaving have to use -mllvm -interleave-loops= to get back to the original behavior......

[asbirlea]

Thanks a lot for the suggestion! Sent: http://lists.llvm.org/pipermail/llvm-dev/2019-April/131968.html

My intention was to not make any change visible to clang.
If clang currently sets the DisableUnrollLoops, and llvm will not use that for LoopVectorization, then have clang set LoopsInterleaved to the same value as the one used for unroll loops.
Adding a separate clang flag for interleaving can be a follow-up change.

Please let me know if this sound reasonable and if I misunderstood how/where clang is setting the flags.

[hsaito]

In D61030#1479208, @asbirlea wrote:

Thanks a lot for the suggestion! Sent: http://lists.llvm.org/pipermail/llvm-dev/2019-April/131968.html

My intention was to not make any change visible to clang.
If clang currently sets the DisableUnrollLoops, and llvm will not use that for LoopVectorization, then have clang set LoopsInterleaved to the same value as the one used for unroll loops.

You mean, clang setting EnableLoopInterleaving. If needed, we can make that change in clang first, i.e., before D61030 goes in, right? If so, there are no immediate surprises to clang users, I think.

Adding a separate clang flag for interleaving can be a follow-up change.

Please let me know if this sound reasonable and if I misunderstood how/where clang is setting the flags.

Sounds reasonable to me.

[asbirlea]

You mean, clang setting EnableLoopInterleaving.

Actually I meant DisableUnrollLoops in the PassManagerBuilder. It's currently always set to false, not based on a flag, and I found a single instance where it's value is changed in clang (see clang patch).

I'm happy to make whatever change needed to clang first, but I don't see what that change is. If you could point me to what I've missed, that would be great!

[hsaito]

In D61030#1479532, @asbirlea wrote:

You mean, clang setting EnableLoopInterleaving.

Actually I meant DisableUnrollLoops in the PassManagerBuilder. It's currently always set to false, not based on a flag, and I found a single instance where it's value is changed in clang (see clang patch).

I'm happy to make whatever change needed to clang first, but I don't see what that change is. If you could point me to what I've missed, that would be great!

All I ask is to check whether -fno-unroll-loops and -funroll-loops clang flags affects what the LoopVectorize constructor sees in it's first incoming parameter. Should be easy enough to print that from the constructor to know for sure. If that's done or your confidence level is as high as that, that's all I need. If those flags affects what LoopVectorize gets, I don't want that behavior to change, and I believe that is also your intent.

[asbirlea]

I verified that the two unroll flags propagate to the option set in the PassManagerBuilder. This change + the clang change in D61142 should not make any visible change for clang users.

[hsaito]

In D61030#1483373, @asbirlea wrote:

I verified that the two unroll flags propagate to the option set in the PassManagerBuilder. This change + the clang change in D61142 should not make any visible change for clang users.

Cheers. Added some of the vectorizer oriented developers to the review. Hope they all like the terminology fix done here.

[rengolin]

lib/Transforms/IPO/PassManagerBuilder.cpp

674 ↗

(On Diff #196662)

I may not have understood the problem, but to keep the previous behaviour, souldn't this be:

MPM.add(createLoopVectorizePass(DisableUnrollLoops || !LoopsInterleaved, !LoopVectorize));

[hsaito]

lib/Transforms/IPO/PassManagerBuilder.cpp

674 ↗

(On Diff #196662)

I think, the intent of the patch is to change that (but not for clang until next time around).

http://lists.llvm.org/pipermail/llvm-dev/2019-April/131968.html

[asbirlea]

lib/Transforms/IPO/PassManagerBuilder.cpp

674 ↗

(On Diff #196662)

Per the clang sibling-patch (D61142), the added field is LoopsInterleaved = !DisableUnrollLoops.

The new flag can be controlled in opt, vs DisableUnrollLoops could not.

A separate clang change can add flags such as -floop-interleave, -fno-loop-interleave to control this flag separately from the -fno-unroll-loops. I am not planning to make this addition.

[rengolin]

lib/Transforms/IPO/PassManagerBuilder.cpp

674 ↗

(On Diff #196662)

Ah, makes sense! Thanks!

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

public:
   /// thin LTO backends, for example for the CFI and devirtualization type
   /// tests.
   const ModuleSummaryIndex *ImportSummary = nullptr;
 
   bool DisableTailCalls;
   bool DisableUnrollLoops;
   bool SLPVectorize;
   bool LoopVectorize;
+  bool LoopsInterleaved;
   bool RerollLoops;
   bool NewGVN;
   bool DisableGVNLoadPRE;
   bool ForgetAllSCEVInLoopUnroll;
   bool VerifyInput;
   bool VerifyOutput;
   bool MergeFunctions;
   bool PrepareForLTO;

llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp

 PassManagerBuilder::PassManagerBuilder() {
     OptLevel = 2;
     SizeLevel = 0;
     LibraryInfo = nullptr;
     Inliner = nullptr;
     DisableUnrollLoops = false;
     SLPVectorize = RunSLPVectorization;
     LoopVectorize = EnableLoopVectorization;
-    // FIXME: Add: LoopsInterleaved = EnableLoopInterleaving;
-    // Replace usage of DisableUnrollLoops with LoopsInterleaved when creating
-    // the LoopVectorize pass, to be consistent with the new pass manager.
+    LoopsInterleaved = EnableLoopInterleaving;
     RerollLoops = RunLoopRerolling;
     NewGVN = RunNewGVN;
     LicmMssaOptCap = SetLicmMssaOptCap;
     LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
     DisableGVNLoadPRE = false;
     ForgetAllSCEVInLoopUnroll = ForgetSCEVInLoopUnroll;
     VerifyInput = false;
     VerifyOutput = false;
void PassManagerBuilder::populateModulePassManager(
   MPM.add(createLoopRotatePass(SizeLevel == 2 ? 0 : -1));
 
   // Distribute loops to allow partial vectorization.  I.e. isolate dependences
   // into separate loop that would otherwise inhibit vectorization.  This is
   // currently only performed for loops marked with the metadata
   // llvm.loop.distribute=true or when -enable-loop-distribute is specified.
   MPM.add(createLoopDistributePass());
 
-  MPM.add(createLoopVectorizePass(DisableUnrollLoops, !LoopVectorize));
+  MPM.add(createLoopVectorizePass(!LoopsInterleaved, !LoopVectorize));
 
   // Eliminate loads by forwarding stores from the previous iteration to loads
   // of the current iteration.
   MPM.add(createLoopLoadEliminationPass());
 
   // FIXME: Because of #pragma vectorize enable, the passes below are always
   // inserted in the pipeline, even when the vectorizer doesn't run (ex. when
   // on -O1 and no #pragma is found). Would be good to have these two passes

llvm/trunk/test/Transforms/LoopVectorize/X86/unroll-pm.ll

 ; RUN: opt < %s -O2 -mtriple=x86_64-apple-macosx10.8.0 -mcpu=corei7-avx -force-vector-width=4 -S | FileCheck %s
-; RUN: opt < %s -O2 -mtriple=x86_64-apple-macosx10.8.0 -mcpu=corei7-avx -force-vector-width=4 -disable-loop-unrolling -S | FileCheck %s -check-prefix=CHECK-NOUNRL
+; RUN: opt < %s -O2 -mtriple=x86_64-apple-macosx10.8.0 -mcpu=corei7-avx -force-vector-width=4 -disable-loop-unrolling -interleave-loops=false -S | FileCheck %s -check-prefix=CHECK-NOUNRL
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.8.0"
 ;CHECK-LABEL: @bar(
 ;CHECK: store <4 x i32>
 ;CHECK: store <4 x i32>
 ;CHECK: ret
 ;CHECK-NOUNRL-LABEL: @bar(