- User Since
- Jul 4 2017, 1:39 AM (186 w, 2 d)
Dec 27 2020
Dec 25 2020
Dec 23 2020
LGTM if my latest comments are addressed.
Let's give this big work a try.
Dec 18 2020
Landed as https://reviews.llvm.org/rGf0e3d1d6ca8c
Ok. See D93521
Dec 17 2020
Dec 14 2020
Dec 11 2020
Dec 3 2020
Nov 22 2020
Thanks. I'm working closely with @skatkov and believe he have enough expertise to review fully.
The biggest issue with the current design is that we have to send FAM to the initialization of StandardInstrumentations/PreservedCFGCheckerInstrumentation. This looks asymmetric to the other analysis managers and it would be more reasonable to send FAM or all AMs to the instrumentation callbacks as parameters.
Nov 19 2020
Not needed after NFC https://github.com/llvm/llvm-project/commit/02dda1c by @nikic
Nov 18 2020
Nov 17 2020
Only PassManagerTest.FunctionPassCFGCheckerInvalidateAnalysis does fail.
The other two (PassManagerTest.FunctionPassCFGCheckerWrapped and PassManagerTest.FunctionPassCFGChecker) pass.
Nov 16 2020
added a test
Nov 15 2020
agree. See my comments in D91017.
The patch looks good, but I have some concerns.
Nov 12 2020
Nov 10 2020
I could not create a simple test. One more option to create one is to change the usage of BPI/BFI to request analysis from AM. Then the test can print BPI to show the difference. I'm not sure this big change is worth this small patch. It is not clear why JumpThreading uses its own local version of BPI/BFI.
Nov 8 2020
This looks to be a good change towards simplification. I think originally a map over edges was introduced by @atrick back in 2011 in https://github.com/llvm/llvm-project/commit/49371f3f33788 as it could be sparse. But with the commit https://gitlab.azulsystems.com/dev/orca/commit/8138487468e22 we imposed an important restriction on setting outgoing edge probabilities at once for one block. This allowed correctness checks and further improvements like this.
Nov 5 2020
Oct 29 2020
EmitGEPOffset() is used by instcombine and we do not see the redundant add 0, X instruction because it is immediately optimized out in the same iteration. It prevents a simple lit test for this patch. On the other hand you can see the fix in InstCombinerImpl::OptimizePointerDifference() which changes the code to expect X in place of add 0, X and there is a related test (test/Transforms/InstCombine/sub-gep.ll), which I believe is enough for this small improvement.
Keep in mind that EmitGEPOffset() could be used in other custom passes that might not require subsequent instcombine to run.
Oct 27 2020
Oct 12 2020
I'm still reading. The patch is long but I'd like to avoid splitting it into pieces that might result in massive tests changes (back and forth). Now the test changes seem to be compact.
It would be great if someone could join the reviewing efforts.
Sep 13 2020
Sep 11 2020
RecursivelyDeleteTriviallyDeadInstructions() call is wrapped with resetIteratorIfInvalidatedWhileCalling().
Ok, but it will be less efficient.
Sep 10 2020
Addressed Fedor's comments.
Sep 9 2020
Fedor, could you please review the New Pass Manager part of this patch as I addressed your comments?
Sep 4 2020
I see no impact on CPU time with the following timed runs (both with and without assertions):
$ time opt -O3 FILE.bc --disable-output -verify-cfg-preserved=VCP
Sep 3 2020
a minor fix in BBGuard.
Implemented custom BBGuard class instead of PoisoningVH which does not track poisoning in release builds.
Removed DEBUG_TYPE and LLVM_DEBUG uses. Unexpected gaph diffs are printed unconditionally.
I will provide performance data soon.
I will collect performance data and fix for release builds.
Sep 2 2020
No further complains from me.
lgtm if the other reviewers' comments are addressed.
Sep 1 2020
Defined a separate class named CFG.
CFG defines the notion PreserveCFG for the multigraph of BasicBlocks with unordered successors.
CFG can safely track and print CFG changes. To guard CFG from accessing deleted blocks PoisoningVH is used and is extended with one getter.
The cfg diff printing function is rewritten.
Aug 30 2020
Aug 27 2020
Aug 26 2020
Aug 23 2020
Aug 21 2020
Aug 20 2020
Aug 13 2020
Aug 6 2020
ok. As I see, both failures have been fixed. Thanks @Meinersbur for fixing polly/test/Isl/Ast/alias_checks_with_empty_context.ll
Aug 5 2020
Aug 4 2020
Thanks. I should have tested some more than just llvm.
Aug 3 2020
Reverted NodeOrderMap from SmallDenseMap to DenseMap.
changed type of SuccOrder to SmallDenseMap<NodePtr, unsigned, 16>.