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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

More comments. Did you run this on the test suite with the new PM and knowledge retention enabled? If it passes there I think we can merge this after this set of comments is through.



================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:315
+
+    Instruction *InsertPt = IsFirst ? BB->getFirstNonPHI() : *Begin;
+    for (IntrinsicInst *I : make_range(Begin, End)) {
----------------
Tyker wrote:
> 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.
I failed to see how the second loop adjusts the position. The comment helps, thanks!


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:329
+      }
+    }
+    if (InsertPt->comesBefore(*Begin))
----------------
Tyker wrote:
> jdoerfert wrote:
> > What happens to the conditions (operand 0)? 
> i am not sure what you mean.
You seem to only check if the `WasOn` part of the bundle comes before the current insertion point and not the condition (operand 0) of the assume.

We need to make sure we don't move:
```
%cond = ...
call llvm.assume(%cond) [align(%arg, 32)]
```


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:228
+      : F(F), AC(AC), DT(DT), C(C),
+        IgnoreTag(C.getOrInsertBundleTag("ignore")) {}
+
----------------
"ignore" should be a constexpr value in some header and everyone should use it.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:245
+  /// or an other assume. This can when valid update an existing knowledge in an
+  /// attribute or an other assume.
+  void dropRedundantKnowledge() {
----------------
I think the second sentence is somehow broken.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:289
+          }
+        }
+        auto &Lookup = Knowledge[{RK.WasOn, RK.AttrKind}];
----------------
I'm always confused by the "context instruction" thing. Does it mean before or after that instruction? I mean, if `&*F.getEntryBlock().getFirstInsertionPt()` may not return is this doing the right thing? Do we have a test?


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:306
+      }
+    }
+  }
----------------
I'm a little afraid this is non-deterministic. It probably depends on the AC->assumptions and the containers/maps you use. WDYT?


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:388
+    }
+  }
+};
----------------
`++SplitIt`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78614