[llvm-commits] [llvm] r160796 - in /llvm/trunk: include/llvm/CodeGen/Passes.h include/llvm/InitializePasses.h lib/CodeGen/CMakeLists.txt lib/CodeGen/EarlyIfConversion.cpp lib/CodeGen/MachineTraceMetrics.cpp lib/CodeGen/MachineTraceMetrics.h

[Jakob Stoklund Olesen]

On Jul 27, 2012, at 4:07 PM, Andrew Trick <atrick at apple.com> wrote:
> 
> +    // Don't leave CurLoop.
> +    if (LB.Loops->getLoopFor(To) != LB.CurLoop)
> +      return false;
> 
> I don't get this. Traces can overlap, so the current trace can include an inner loop iteration, from the perspective of the loop preheader or loop exit.

Yes, confining to just one loop is a simplification.

It should be relatively simple to constrain traces so they never leave a loop as seen from the center block. They could be allowed to enter loops.

The whole trace building and invalidation mechanism is still a bit shaky. I'll generalize this when it's more stable.

> +  /// Invalidate cached information about MBB. This must be called *before* MBB
> +  /// is erased, or the CFG is otherwise changed.
> 
> The cautionary comment on invalid() is good! It seems like it would be easy to catch the inconsistent CFG problem with a verifyAnalysis that checked trace pred/succ edges.

Good idea.

> This comment is mysterious:
> 
> +    // Ignore invalidated predecessors. This never happens on the first scan,
> +    // but if we rejected this predecessor earlier, it won't be revalidated.
> 
> AFAICT you should assert that the predecessor is valid. You need to reevaluate it before you can pick correctly after something changes.

Yes, I think this is a papered-over bug.

> It might also help to clarify that there are two things being invalidated, trace-level information vs. block-local information. It helps understand what's happening to know that block-local information will only be invalidated for a single block.

I'll add a comment.

> There's also no reason we shouldn't be able to add blocks, as long as the traces reaching new edges are invalidated first. The current invalidate API isn't quite right for that, because we don't want to invalidate any block-local info. Just something to think about for now.

Do you mean critical edge splitting?

> static isFree()... ugh.

static bool isSSANonsense()?

/jakob