[PATCH] D152358: [CGP] Remove operand of llvm.assume more aggressively.

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

Thanks a lot for review. I'll update a patch as soon as we agreed on terminology (Closure) and description of FindClosureOfWouldBeTriviallyDeadInstructions function.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:707
 
+// Returns ture if transitive closure of instruction I is trivially dead
+// if no uses. In this case Closure contains this transitive closure. Returns
----------------
arsenm wrote:
> sentence sounds like it's missing a word here?
Well.. Let me try to re-phrase it as follows, please review is it better?

The utility function collects all transitively reachable users of I. If all users would be trivially dead in case they
would not have users then this function returns true and Closure contains I with all found users.
Otherwise function returns false and Closure content is not specified. 
For the purpose of this function llvm.assume is considered ad trivially dead instruction.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:711
+// llvm.assume is considered as trivial dead instruction.
+static bool FindClosureOfWouldBeTriviallyDeadInstructions(
+    Instruction *I, SmallPtrSetImpl<Instruction *> &Closure,
----------------
arsenm wrote:
> arsenm wrote:
> > Start with lowercase
> I'm not sure what closure means in this context
Will do.

Closure means transitive closure of users of instruction I In other words I, its users, users of users and so on.

Any suggestion for better term?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:715
+  assert(wouldInstructionBeTriviallyDead(I, TLInfo) && "expect trivially dead");
+  Closure.insert(I);
+  // Simple Case. no users.
----------------
arsenm wrote:
> Do this after the early exit?
No, early exit is a simple case when there is no users, so closure consists of 1 instruction I and I should return it in Closure for further erasing.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:739-740
+
+// For the given trivially dead instruction if no uses, collect all instruction
+// instruction which can be removed together with it.
+static void FindInstructionsToRemoveWithAssumeOperand(
----------------
arsenm wrote:
> How is this different from RecursivelyDeleteTriviallyDeadInstructions?
RecursivelyDeleteTriviallyDeadInstructions will delete instruction only if has no users, while this patch will find group of instructions which is connected to each other (see tests below, induction variable case) and remove them together is none of these instructions has users outside of this group and all of them would be trivially dead.

Also this function consider llvm.assume as trivially dead instruction. This is for the case when induction variable is used in two different assumes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152358/new/

https://reviews.llvm.org/D152358