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

[Jakob Stoklund Olesen]

On Jul 5, 2012, at 1:08 PM, Andrew Trick <atrick at apple.com> wrote:

> 
> On Jul 3, 2012, at 5:09 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
> 
>> Author: stoklund
>> Date: Tue Jul  3 19:09:54 2012
>> New Revision: 159694
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=159694&view=rev
>> Log:
>> Add an experimental early if-conversion pass, off by default.

> You're running this after TailDup, probably because you want MachineDCE to run first. I'm not sure when EarlyTailDup is currently kicking in, but we generally want to try if-converting a pass before tailduping it.

IIRC, EarlyTailDup is mostly focused on indirect branches. It definitely won't touch return blocks because the epilog size is unknown.

> Ideal pass order is: DCE, IfCvt, TailDup, ReDCE only where TailDup did something.

I didn't put a lot of thought into the pass ordering. I don't think DCE has a lot to do normally, so it is possible we can simply go: IfCvt, TailDup, DCE.

> +for (MachineFunction::iterator MFI = MF.begin(), MFE = MF.end(); MFI != MFE;
> +       ++MFI)
> +    if (tryConvertIf(MFI))
> 
> How does this iterator work if you're deleting blocks? Maybe a comment.

It works because the if-conversion never deletes the head block. I'll add a comment.

> I think it would be nice if CFG transforms don't depend on arbitrary block layout order.
> Instead of using a worklist, would you consider a postorder domtree walk? That might solve two problems at once (stable order + nested if-conversion). The parent domtree node won't be invalidated by the if-conversion itself.

That would be fine, but it is just a performance optimization. The current implementation simply iterates to convergence, so the initial ordering shouldn't matter.

When I add heuristics, that may change. I can imagine the order affecting heuristics.

> Speaking of which, Are you considering incremental DomTree update? It should be easy for if-conversion.

Yes, I considered it, as well as LoopInfo update. These analyses are not live in the current pass order, which is why I ignored them. I agree that it should be easy.

> +  if (Tail->pred_size() != 2)
> +    return false;
> 
> That doesn't look right. How can we handle simple nested cases like this?
> 
> A
> |\
> B \
> |\ \
> C E F
> | | |
>  \|/
>   D

I chickened out. It is perfectly possible to handle this case, but it must be done 'inside out':

- Convert B -> C/E -> D. Rewrite the PHI in D instead of replacing it.
- Convert A -> B/F -> D. This time eliminate the PHI in D, and possibly join D with A.

I will look into this.

> Taking it a step further, don't we want to handle simple "or" conditions?
> 
> A
> | \
> B  F
> |\  \
> C E  |
> |  \/
> |  G
> | /
> D

Possibly, it really depends on what turns out to be profitable. I wasn't really expecting that converting complicated structures would be profitable.

I think we should return to this when heuristics are in place.

> You're looking for simple patterns with single block on a path, so assuming prior passes have all aggressively merged fall-through blocks. Maybe this assumption should be commented.

I am not sure what you mean here. The only assumption made about layout is whether the tail block should be merged with the head block, or an unconditional branch is required.

> 
> + bool SSAIfConv::canSpeculateInstrs(MachineBasicBlock *MBB) {
> ...
> +  for (MachineBasicBlock::iterator I = MBB->begin(),
> +       E = MBB->getFirstTerminator(); I != E; ++I) {
> 
> Dumb question: When is it safe to completely ignore MI terminators like this? Are we guaranteed that they don't write physical registers or have any other side effects? This would be a good place for a comment.

Terminators definitely can't write virtual registers, as Hal learned recently.

We don't really have hard rules about this stuff, current targets simply don't have side-effecting terminators. I imagine it would create all sorts of problems.

/jakob

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120705/81e2e560/attachment.html>