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

[Tyker via Phabricator via llvm-commits]

Tyker added a comment.

on a separate note.
i started testing on real-word code with enable-knowledge-retention.
after a few fixes the compiler didn't crashed on any application i tried. and there doesn't seem to be any miss-compile either.
however the compile time cost is very high, originally around +15% after some tweaking i lowered it to 9%.

from what i saw the cause of the slowdown is cause by salvageKnowledge that generate lots of extra instructions and many part of the optimizer scale on the number of instruction.
here is the time taken by the parts i changed/added in the tweaked version:
0.43% in salvageKnowledge
0.01% in assumes Queries
0.70% in the assume simplify pass (i added it in 4 places in the pipeline as part of tweaks)
although they could probably be optimized, they are not a big part of the slowdown.

i still have a few idea to try out and there is also lots of arbitrary decision i made during development that could be guided by measurement.
so the current overhead can surely be lowered but i don't think i can get it to a reasonable level without significantly limiting the preserved information.

i didn't start measuring the runtime impact but i don't expect it to be good yet. since droppable uses are not widely in place.

any thought ?



================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:315
+
+    Instruction *InsertPt = IsFirst ? BB->getFirstNonPHI() : *Begin;
+    for (IntrinsicInst *I : make_range(Begin, End)) {
----------------
jdoerfert wrote:
> 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!
i added a comment below on where the position gets adjusted.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:329
+      }
+    }
+    if (InsertPt->comesBefore(*Begin))
----------------
jdoerfert wrote:
> 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)]
> ```
i changed buildMapping such that i can filter assume that don't have true as argument. this way we don't affect them.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:228
+      : F(F), AC(AC), DT(DT), C(C),
+        IgnoreTag(C.getOrInsertBundleTag("ignore")) {}
+
----------------
jdoerfert wrote:
> "ignore" should be a constexpr value in some header and everyone should use it.
i added IgnoreBundleTag but sadly some uses of "ignore" in IR can't be replaced by it. since it resides in Analysis


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:289
+          }
+        }
+        auto &Lookup = Knowledge[{RK.WasOn, RK.AttrKind}];
----------------
jdoerfert wrote:
> 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?
i don't know either but this is covered by test4A and it seems to be before the context instruction or the test would fail.

the test verifies that we do not replace the assume with function attributes when the first instruction is a may throw.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:306
+      }
+    }
+  }
----------------
jdoerfert wrote:
> I'm a little afraid this is non-deterministic. It probably depends on the AC->assumptions and the containers/maps you use. WDYT?
this does depend on the AC->assumptions ordering. but i don't think it depends on the map ordering since we only do lookup in the map. 

i changed the iteration order such that it is deterministic. even if AC->assumptions isn't.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:342
+        if (!isGuaranteedToTransferExecutionToSuccessor(&*It)) {
+          InsertPt = It->getNextNode();
+          break;
----------------
here is the position adjustment in the second loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78614