Hey Nadav, super excited about this.<div><br></div><div>Recently, we tried to roll out some significant improvements to GCC's stack slot re-use, and discovered a really terrifying number of bugs in user code stemming from this. I would like to make sure that when using tools like ASan, this logic is disabled, and ASan can correctly detect code using an alloca after its lifetime has ended. Without this, it will be very hard to track down mis-compiles introduced here. Adding Kostya as I'm not familiar with the exact current state of ASan and lifetime of alloca instrumentation.</div>
<div><div><br></div><div><br></div><div>Also, some code comments inline:</div><div><br></div><div><div>Index: test/CodeGen/X86/StackColoring.ll</div><div>===================================================================</div>
<div>--- test/CodeGen/X86/StackColoring.ll<span class="" style="white-space:pre"> </span>(revision 0)</div><div>+++ test/CodeGen/X86/StackColoring.ll<span class="" style="white-space:pre"> </span>(revision 0)</div><div>@@ -0,0 +1,132 @@</div>
<div>+; RUN: llc < %s | FileCheck %s --check-prefix=COLOR</div><div>+; RUN: llc < %s -no-stack-coloring | FileCheck %s --check-prefix=NOCOLOR</div><div>+</div><div>+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"</div>
<div>+target triple = "x86_64-apple-macosx10.8.0"</div><div>+</div><div>+;COLOR: subq $136, %rsp</div><div>+;COLOR: subq $144, %rsp</div><div>+;COLOR: subq $208, %rsp</div><div>+;COLOR: subq $112, %rsp</div>
<div>+</div><div>+;NOCOLOR: subq $264, %rsp</div><div>+;NOCOLOR: subq $272, %rsp</div><div>+;NOCOLOR: subq $400, %rsp</div><div>+;NOCOLOR: subq $400, %rsp</div></div><div><br></div><div>Can you put the filecheck assertions inside the functions in question, and add anchoring assertions for the function name? That makes the test easier for me to read, debug, and update.</div>
<div><br></div><div><div>Index: include/llvm/CodeGen/ISDOpcodes.h</div><div>===================================================================</div><div>--- include/llvm/CodeGen/ISDOpcodes.h<span class="" style="white-space:pre"> </span>(revision 162761)</div>
<div>+++ include/llvm/CodeGen/ISDOpcodes.h<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -637,6 +637,10 @@</div><div> ATOMIC_LOAD_UMIN,</div><div> ATOMIC_LOAD_UMAX,</div><div> </div><div>
+ /// This corresponds to the LIFETIME intrinsics. The first operand</div><div><br></div><div>I would write this ".. to the @llvm.lifetime intrinsics. ..." or otherwise differ from the style of the ISD node enumerators...</div>
<div><br></div><div>+ /// is the chain and the second operand is the alloca pointer.</div><div>+ LIFETIME_START, LIFETIME_END,</div><div>+</div></div><div><br></div><div><br></div><div><div>Index: lib/CodeGen/StackColoring.cpp</div>
<div>===================================================================</div><div>--- lib/CodeGen/StackColoring.cpp<span class="" style="white-space:pre"> </span>(revision 0)</div><div>+++ lib/CodeGen/StackColoring.cpp<span class="" style="white-space:pre"> </span>(revision 0)</div>
<div>@@ -0,0 +1,593 @@</div><div>+//===-- StackColoring.cpp -------------------------------------------- ----===//</div><div>+//</div><div>+// The LLVM Compiler Infrastructure</div><div>+//</div><div>+// This file is distributed under the University of Illinois Open Source</div>
<div>+// License. See LICENSE.TXT for details.</div><div>+//</div><div>+//===----------------------------------------------------------------------===//</div><div>+//</div><div>+// This pass implements the stack-coloring optimization that looks for</div>
<div>+// lifetime markers machine instructions (LIFESTART_BEGIN and LIFESTART_END),</div><div>+// which represent the possible lifetime of stack slots. It attempts to</div><div>+// merge disjoint stack slots and reduce the used stack space.</div>
<div>+// NOTE: This pass is not StackSlotColoring, which optimizes spill slots.</div><div><br></div><div>Can you add some notes here about the future work? Specifically as it relates to the other pass.</div><div><br></div>
<div>Also, would it be possible to rename one or the other pass to make the differences between them more obvious to the reader, and not require this note? (clearly as a follow-up or preparatory commit)</div></div><div><br>
</div><div><div>+namespace {</div><div>+class StackColoring : public MachineFunctionPass {</div><div><br></div><div>Doxygen comments here and throughout please.</div><div><br></div><div>+ MachineFrameInfo *MFI;</div><div>
+ MachineFunction *MF;</div><div>+</div><div>+ typedef DenseMap<MachineBasicBlock*, BitVector> SlotsPerBlock;</div><div>+</div><div>+ // Holds a list of active slots per basic block.</div><div>+ SlotsPerBlock BEGIN;</div>
<div>+ SlotsPerBlock END;</div><div>+ SlotsPerBlock LIVE_IN;</div><div>+ SlotsPerBlock LIVE_OUT;</div><div><br></div><div>These should definitely have very explanatory comments. Also, as this is new code, please follow the coding convention for naming and make these CamelCase.</div>
<div><br></div><div>I'm surprised at having four separate dense maps here. What's the rationale behind that? My (admittedly naive, so your explanation may be all that I need) base line assumption would be that this would either call for a single map from the block pointer to a struct of four BitVectors, or that a better solution would be to put both a flag (indicating begin, end, live_in, and live_out) and the block pointer in the key and have a single map. The former seems most appropriate if the expectation is that essentially every block will have all four of these, and that they are all four accessed equally frequently. The latter would seem best if often not all four of these are present for any particular block. What you have only really makes sense to me if we always have all four, but one of these is accessed *much* more often than the others, and thus we want greater locality for that densemap? Anyways, a comment about this would help a lot I suspect, regardless of the final representation.</div>
<div><br></div><div>Also, how large are these bitvectors? I worry a bit about how happily DenseMap will copy the values.</div><div><br></div><div>+</div><div>+ // Mapping between basic blocks and a serial number.</div><div>
+ DenseMap<MachineBasicBlock*, int> BasicBlocks;</div><div>+ SmallVector<MachineBasicBlock*, 8> BasicBlockNumbering;</div><div>+</div><div>+ // The intervals to describe the active ranges for each slot.</div>
<div>+ SmallVector<LiveInterval*, 16> Intervals;</div><div>+ VNInfo::Allocator VNInfoAllocator;</div><div>+ SlotIndexes* Indexes;</div><div>+</div><div>+ // The list of lifetime markers found.</div><div>+ SmallVector<MachineInstr*, 8> Markers;</div>
<div>+</div><div>+ struct SlotSizeSorter {</div><div><br></div><div>It's not at all clear to me how this sorter is used... doxygen comments will likely help here.</div><div><br></div><div>+ MachineFrameInfo *MFI;</div>
<div>+ SlotSizeSorter(MachineFrameInfo *M) : MFI(M) { }</div><div><br></div><div>Prevailing style uses the exact same name as the member for the initializing parameter.</div><div><br></div><div>+ bool operator()(int LHS, int RHS) {</div>
<div>+ if (LHS == -1) return 0;</div><div>+ if (RHS == -1) return 1;</div><div><br></div><div>0 and 1? How about false and true?</div><div><br></div><div>Is this still a SWO? Should these just be asserts?</div><div>
<br></div><div>+ size_t LSize = MFI->getObjectSize(LHS);</div><div>+ size_t RSize = MFI->getObjectSize(RHS);</div><div>+ return LSize > RSize;</div><div><br></div><div>Why the variables?</div><div>
<br></div><div>+ }</div><div>+};</div></div><div><br></div><div>The indent for these two curlies is wrong.</div><div><br></div><div><div>+private:</div><div>+ /// Debug.</div><div>+ void dump();</div><div>+</div><div>+ /// Removes all of the lifetime marker instructions from the function.</div>
<div><br></div><div>How do you feel about putting all of the doxygen comments at the implementation site since this isn't part of the interface, and just a private helper function?</div><div><br></div><div>+ bool RemoveAllMarkers();</div>
<div><br></div><div>Please use the new convention (camelCase) for method names throughout.</div><div><br></div><div>+</div><div>+ /// Scan the machine function and find all of the lifetime markers.</div><div>+ /// Record the findings in the BEGIN and END vectors.</div>
<div><br></div><div>We don't have BEGIN and END vectors, we have dense maps of bitvectors? Maybe this just needs more clarification in the datastructure represented by those vectors.</div><div><br></div><div>+ /// returns the number of markers found.</div>
<div><br></div><div>Use doxygen \returns?</div><div><br></div><div>+ unsigned CollectMarkers(unsigned NumSlot);</div><div>+</div><div>+ /// Perform the dataflow calculation and calculate the lifetime for each of</div><div>
+ /// the slots, based on the BEGIN/END vectors. Set the LIVE_IN and LIVE_OUT</div><div>+ /// vectors that represent which stack slots are live coming in and out</div><div>+ // blocks.</div><div>+ void CalculateLocalLiveness();</div>
<div>+</div><div>+ /// Construct the LiveIntervals for the slots.</div><div>+ void CalculateLiveIntervals(unsigned NumSlots);</div><div>+</div><div>+ /// Go over the machine function and change instructions which use stack</div>
<div>+ /// slots to use the joint slots.</div><div>+ void RemapInstructions(DenseMap<int, int> &SlotRemap);</div><div>+</div><div>+ /// Map entries which point to other entries to their destination.</div><div>
+ /// A->B->C becomes A->C.</div><div>+ void ExpungeSlotMap(DenseMap<int, int> &SlotRemap, unsigned NumSlots);</div><div>+};</div></div><div><br></div></div>