[PATCH] D46706: [PM/LoopUnswitch] Support partial trivial unswitching.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 8 inline comments as done.
chandlerc added a comment.

Ok, after some discussion, I'm pretty convinced we just need to stop simplifying here.

I was hoping to do at least *some* simplification, but there really is no good middle ground and so I think its better to *completely* nuke this.

Unfortunately that is going to require ... some more work.

1. I think the current pass pipeline doesn't have a good cleanup pass scheduled. I don't think we even have the *right* cleanup pass here, but I'll poke around at some of the nascent ones. So first step will be that I need to go build / finish / polish a good instruction cleanup loop pass. And then I'll need to add it to the main pipelines. Prepare for patches to that effect.

2. I'll then need to do the removal of all simplifying here in a separate patch and update testing to reflect it. Specifically, when we *only* do trivial unswitching, we were falling through to try non-trivial unswitching immediately. We will have to not do that, and instead re-add the now mutated loop to the pass manager to re-visit (much like we do with non-trivial unswitching) and rely on it then iterating for us. This will ... not be a somewhat surprisingly significant behavior change. I think its good, but its worth noting. This will essentially make loop-unswitch a fixed-point pass in the pass pipeline, but it will do so using the pass manager. I'm not aware of any other passes that currently do this. Anyways, brave new world. The pipeline will now be ... truly dynamic.

3. Then I can return to this patch and land it w/o added simplification.

Make sense?

I've updated the code here to reflect what I think it will end up looking like. I can't update the tests yet, will do that when I get back to #3.

I'll also probably pull the recursive deletion API into one of the patches in #1 -- would you be OK with that going in on its own? I don't have a good way of testing it that way though. Otherwise, I'll let it float around and try to figure out when/if I need it.

-Chandler



================
Comment at: llvm/include/llvm/Analysis/Utils/Local.h:144
 
+/// Delete all of the instructions in the provided vector, and all other
+/// instructions that deleting these in turn causes to be trivially dead.
----------------
sanjoy wrote:
> sanjoy wrote:
> > Why not s/provided vector/`DeadInsts`?
> Wasn't done?
Oh, sorry. I changed it in the last paragraph, but not the top one.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:228
+  if (!SimplifyBlockSet.empty()) {
+    // If we still have blocks to process it is because we have instructions
+    // across multiple blocks. We want to ensure we simplify defs before their
----------------
sanjoy wrote:
> Maybe this can go in a different "loop instruction simplify" pass?  Just above you say "the only real goal of this is to do very basic cleanup of unswitched conditions.  We don't need powerful tools here. A proper pass can be scheduled to do more comprehensive cleanup." :)
> 
> I'm worried that doing all of this in a single pass makes it less granular and harder to test -- we only get to the see the IR after all of this cross-block simplification has happened, which means it is harder to figure out what exactly loop unswitching did.
After a bunch of discussion, there really isn't even a good middle ground here.

I think we have to rip *all* of the cleanup out. This is going to make things harder to do as it changes how the current pass behaves. See my top-level response.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:447
+    Instruction &I = *DeadInsts.pop_back_val();
+    salvageDebugInfo(I);
 
----------------
sanjoy wrote:
> Can we end up doing a double free here?  Like
> 
> ```
> %a = add %p, 1
> %b = add %a, 1
> ```
> 
> and `DeadInsts` is [`%a`,`%b`] -- we'll add `%a` back to the worklist after visiting `%b` and then `eraseFromParent` `%a` twice.
> 
> We could add a precondition here to avoid this, but it seems better to just handle this case.
This is already in the precondition documentation... Specifically that the instructions in the vector must have no uses.

I've added an assert to ensure this holds. I'm happy to widen the contract if you think users would benefit from it though.


Repository:
  rL LLVM

https://reviews.llvm.org/D46706