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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
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:
> Why not s/provided vector/`DeadInsts`?
Wasn't done?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:208
+
+  // While we have a single block in need of simplifying, we can just walking
+  // the instructions in that block and simplify the instructions in the order
----------------
"just walk"


================
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
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:447
+    Instruction &I = *DeadInsts.pop_back_val();
+    salvageDebugInfo(I);
 
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:453
+      Value *OpV = OpU.get();
+      OpU = nullptr;
 
----------------
I think `OpU.set(nullptr)` is more readable here.  Otherwise this reads like we're just modifying a local variable.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/trivial-unswitch.ll:511
+  %cond_or4 = or i1 %cond_or3, %cond4
+  br i1 %cond_or4, label %loop_exit, label %do_something
+; CHECK:       loop_begin:
----------------
chandlerc wrote:
> sanjoy wrote:
> > I think we need to test a few more cases here:
> > 
> >  - Branch on chain of `and`s
> >  - Branch on mix of `and`s and `or`s and possibly other operations
> So, we test branching on an `and` above. There is no different logic in handling N `and` instructions vs. N `or` instructions, so I didn't add that test as it didn't seem to add value compared to these two.
> 
> I can definitely mix some non `or` operations into this chain. Would that be enough coverage? Just trying to understand what the goal is of the added tests.
> 
> While I was adding these tests, it actually exposed a weakness in the instruction simplification we do here. Fixing it proved... a bit tricky. I've added somewhat complex logic so that when we need to simplify multiple different invariants we can correctly simplify defs before uses. However, now I need to add a better test to cover the nasty case for simplifying -- when we need to simplify around a diamond in the CFG. I'm going ahead and uploading the patch while I craft a test case for that so you can yell if I'm making this way harder than it needs to be.
> Just trying to understand what the goal is of the added tests.

Mostly just making sure that the logic to "gather" the various loop invariant (transitive) operands keeps working as intended.


Repository:
  rL LLVM

https://reviews.llvm.org/D46706