[PATCH] D78614: [AssumeBuilder] Add assume-simplify pass

[Tyker via Phabricator via llvm-commits]

Tyker marked 2 inline comments as done.
Tyker added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:232
+      if (!V)
+        continue;
+      IntrinsicInst *Assume = cast<IntrinsicInst>(V);
----------------
jdoerfert wrote:
> Why `Value*` and how can it be `null`? 
in a past version i was iterating over the assumption cache, this can now be simplified.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:276
+                    Assume, &*F.getEntryBlock().getFirstInsertionPt()))
+              goto update;
+          }
----------------
jdoerfert wrote:
> What is updated here? I first thought the argument attribute is merged into the assume but I cannot figure out how the value is passed. I guess a lambda makes this clear (see also below).
> 
> On second thought, I doubt we can update the llvm.assume, so if this is what you are doing, I doubt it is legal. It works for "bit-wise" attribues like `nonnull` but not for "stateful" ones like `dereferenceable`.
> 
> Thinking this further, we need to distinguish these "stateful" attributes in other places as well. The attributor performs a similar lookup (mistake) right now (I think).
the update code changed in the last update.

what is getting updated is the other knowledge either the argument of the function or the other assume. this only occurs when there isValidAssumeForContext returns true in both direction, so the assume are in equivalent contexts.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:315
+
+    Instruction *InsertPt = IsFirst ? BB->getFirstNonPHI() : *Begin;
+    for (IntrinsicInst *I : make_range(Begin, End)) {
----------------
jdoerfert wrote:
> Why can we move assumptions up to the first PHI?
when generating a new assume this code tries to put that assume as close as possible to the beginning of the BasicBlock. such that isValidAssumeForContext Queries on that assume can be resolved with a simple comesBefore instead of a loop of isGuaranteedToTransferExecutionToSuccessor.

at this point we know that all assumes from Begin to End can be merged because there is no instruction between them for which isGuaranteedToTransferExecutionToSuccessor doesn't return true, so if the first assume is reached all assumes in the range will be.

in the first loop, InsertPt is adjusted to keep the assume dominated by all values it uses.

in the second loop, if the insertion is still above Begin after the first loop we look how high can it be and place it there.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:329
+      }
+    }
+    if (InsertPt->comesBefore(*Begin))
----------------
jdoerfert wrote:
> What happens to the conditions (operand 0)? 
i am not sure what you mean.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:380
+  }
+};
+
----------------
jdoerfert wrote:
> We need comments on these methods explaining what they do.
i added comments on functions. hope its enought 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78614