Remove optnone/opt-bisect check from StackColoring

[andrew.w.kaylor]

Summary

This removes the skipFunction() call in the StackColoring pass that allows the pass to be skipped for opt-bisect and "optnone" function attribute handling.

This pass was calling skipOptnoneFunction() before I made the opt-bisect changes, but if a function reaches this pass with lifetime.start/lifetime.end intrinsics in place and the pass is skipped code generation will fail. This pass is not called at opt-level zero, but the lifetime intrinsics are never translated to machine instructions at opt-level zero.

It would be possible to have the pass simply remove the intrinsics and do nothing else if the skipFunction() call returns 'true' but I don't like the idea of having a pass that changes the IR in a different way when it is told not to change the IR. We could always re-visit that later if the need arises.

[probinson]

Hmmm. In the optnone case, presumably we're not running that pass that inserts the lifetime markers; if there are no markers, and this pass runs, naively it looks like this would be a no-op pass. So, from the optnone perspective, this change would not be a problem.

But if you're bisecting, there might be markers, and if the bisection point is between the insertion and this pass, then later on codegen will fail. This change means, if the bisection point is between the insertion and this pass, then this pass runs anyway.

In effect this becomes an always-run pass, and whether it does anything depends on whether the marker-insertion pass runs. Makes the bisection less ideal, but I guess we can live with that.

test/Other/opt-bisect-legacy-pass-manager.ll

157 ↗

(On Diff #57871)

This wants some commentary, as each hunk of this test is doing something particular and this new hunk isn't doing the same thing as the previous hunk.

[andrew.w.kaylor]

In D20453#441338, @probinson wrote:

Hmmm. In the optnone case, presumably we're not running that pass that inserts the lifetime markers; if there are no markers, and this pass runs, naively it looks like this would be a no-op pass. So, from the optnone perspective, this change would not be a problem.

But if you're bisecting, there might be markers, and if the bisection point is between the insertion and this pass, then later on codegen will fail. This change means, if the bisection point is between the insertion and this pass, then this pass runs anyway.

In effect this becomes an always-run pass, and whether it does anything depends on whether the marker-insertion pass runs. Makes the bisection less ideal, but I guess we can live with that.

The lifetime start call can be inserted by AtomicExpand, which isn't skipped by optnone or opt-bisect, or by SROA, which is run very early, or by the function inliner, which can't be skipped for the AlwaysInline pass.

[thanm]

Hi Andy,
I think I agree with probinson about the need for removing the markers in the opt-none case. I think the most expedient thing to do would be to make the skipFunction() check at the same point in the code where "DisableColoring" is checked (line 1008), this way the "removeAllMarkers()" helper will be called on the way out. This has the minor compile-time drawback that there is some analysis done to get to this point, but that seems like a minor concern for bisections purposes.
Thanks, Than

[probinson]

In D20453#441677, @thanm wrote:

Hi Andy,
I think I agree with probinson about the need for removing the markers in the opt-none case. I think the most expedient thing to do would be to make the skipFunction() check at the same point in the code where "DisableColoring" is checked (line 1008), this way the "removeAllMarkers()" helper will be called on the way out. This has the minor compile-time drawback that there is some analysis done to get to this point, but that seems like a minor concern for bisections purposes.
Thanks, Than

I like Than's idea. For either bisection or optnone, the compile-time "drawback" is pretty irrelevant.

[andrew.w.kaylor]

Sounds good to me. I'll upload changes for final review as soon as my local testing finishes. It looks like I still need to take that line out of the optnone-llc test because the pass aborts for other reasons before calling skipFunction. Otherwise, this looks like a very clean solution.

[andrew.w.kaylor]

Implemented suggested revisions.

[thanm]

LGTM

llvm/trunk/lib/CodeGen/StackColoring.cpp

if (SlotRemap.count(i)) {
         Target = SlotRemap[Target];
         SlotRemap[i] = Target;
       }
     }
   }
 }
 
 bool StackColoring::runOnMachineFunction(MachineFunction &Func) {
-  if (skipFunction(*Func.getFunction()))
-    return false;
-
   DEBUG(dbgs() << "********** Stack Coloring **********\n"
                << "********** Function: "
                << ((const Value*)Func.getFunction())->getName() << '\n');
   MF = &Func;
   MFI = MF->getFrameInfo();
   Indexes = &getAnalysis<SlotIndexes>();
   SP = &getAnalysis<StackProtector>();
   BlockLiveness.clear();
for (int i=0; i < MFI->getObjectIndexEnd(); ++i) {
     DEBUG(dbgs()<<"Slot #"<<i<<" - "<<MFI->getObjectSize(i)<<" bytes.\n");
     TotalSize += MFI->getObjectSize(i);
   }
 
   DEBUG(dbgs()<<"Total Stack size: "<<TotalSize<<" bytes\n\n");
 
   // Don't continue because there are not enough lifetime markers, or the
   // stack is too small, or we are told not to optimize the slots.
-  if (NumMarkers < 2 || TotalSize < 16 || DisableColoring) {
+  if (NumMarkers < 2 || TotalSize < 16 || DisableColoring ||
+      skipFunction(*Func.getFunction())) {
     DEBUG(dbgs()<<"Will not try to merge slots.\n");
     return removeAllMarkers();
   }
 
   for (unsigned i=0; i < NumSlots; ++i) {
     std::unique_ptr<LiveInterval> LI(new LiveInterval(i, 0));
     LI->getNextValue(Indexes->getZeroIndex(), VNInfoAllocator);
     Intervals.push_back(std::move(LI));

llvm/trunk/test/Feature/optnone-llc.ll

 ; LLC-Ox-DAG: Skipping pass 'Branch Probability Basic Block Placement'
 ; LLC-Ox-DAG: Skipping pass 'CodeGen Prepare'
 ; LLC-Ox-DAG: Skipping pass 'Control Flow Optimizer'
 ; LLC-Ox-DAG: Skipping pass 'Machine code sinking'
 ; LLC-Ox-DAG: Skipping pass 'Machine Common Subexpression Elimination'
 ; LLC-Ox-DAG: Skipping pass 'Machine Copy Propagation Pass'
 ; LLC-Ox-DAG: Skipping pass 'Machine Instruction Scheduler'
 ; LLC-Ox-DAG: Skipping pass 'Machine Loop Invariant Code Motion'
-; LLC-Ox-DAG: Skipping pass 'Merge disjoint stack slots'
 ; LLC-Ox-DAG: Skipping pass 'Optimize machine instruction PHIs'
 ; LLC-Ox-DAG: Skipping pass 'Peephole Optimizations'
 ; LLC-Ox-DAG: Skipping pass 'Post{{.*}}RA{{.*}}{{[Ss]}}cheduler'
 ; LLC-Ox-DAG: Skipping pass 'Remove dead machine instructions'
 ; LLC-Ox-DAG: Skipping pass 'Tail Duplication'
 
 ; Alternate post-RA scheduler.
 ; LLC-MORE: Skipping pass 'PostRA Machine Instruction Scheduler'
 
 ; Selectively disable FastISel for optnone functions.
 ; FAST:   FastISel is enabled
 ; NOFAST: FastISel is disabled

llvm/trunk/test/Other/opt-bisect-legacy-pass-manager.ll

entry:
   %icmp = icmp ugt i32 %temp, 2
   br i1 %icmp, label %bb.true, label %bb.false
 bb.true:
   %temp2 = call i32 @f2()
   ret i32 %temp2
 bb.false:
   ret i32 0
 }
+
+; This function is here to verify that opt-bisect can skip all passes for
+; functions that contain lifetime intrinsics.
+define void @f4() {
+entry:
+  %i = alloca i32, align 4
+  %tmp = bitcast i32* %i to i8*
+  call void @llvm.lifetime.start(i64 4, i8* %tmp)
+  br label %for.cond
+
+for.cond:
+  br i1 undef, label %for.body, label %for.end
+
+for.body:
+  br label %for.cond
+
+for.end:
+  ret void
+}
+
+declare void @llvm.lifetime.start(i64, i8* nocapture)
+