[analysis] Simplify core analysis code #6034

tlively · 2023-10-20T23:08:39Z

Simplify the monotone analyzer by replacing all the state it used to store in
BlockState with a simple vector of lattice elements. Use simple indices to
refer to both blocks and their associated states in the vector. Remove the
ability for transfer functions to control the initial enqueued order of basic
blocks since that was a leaky abstraction. Replace the worklist with a
UniqueDeferredQueue since that has generally proven to be more efficient in
smiilarly contexts, and more importantly, it has a nicer API. Make miscellaneous
simplifications to other code as well.

Delete a few unit tests that exposed the order in which blocks were analyzed
because they printed intermediate results. These tests should be replaced with
tests of analyses' public APIs in the future.

tlively · 2023-10-20T23:08:52Z

Current dependencies on/for this PR:

main
- PR [analysis][NFC] Create a TransferFunction concept #6033
  - PR [analysis] Simplify core analysis code #6034 👈
    - PR [analysis][NFC] Rename makeLeastUpperBound to join and move it to lattice #6035
      - PR [analysis] Implement a Bool lattice #6036
        
        PR [analysis] Implement an Int lattice #6037
        
        PR [analysis] Add a FullLattice concept and Inverted lattice #6038
        
        PR [analysis] Implement Flat lattice #6039
        
        PR [analysis] Implement a Lift lattice #6040
        
        PR [analysis] Improve lattice fuzzer #6050

This stack of pull requests is managed by Graphite.

Simplify the monotone analyzer by replacing all the state it used to store in `BlockState` with a simple vector of lattice elements. Use simple indices to refer to both blocks and their associated states in the vector. Remove the ability for transfer functions to control the initial enqueued order of basic blocks since that was a leaky abstraction. Replace the worklist with a UniqueDeferredQueue since that has generally proven to be more efficient in smiilarly contexts, and more importantly, it has a nicer API. Make miscellaneous simplifications to other code as well. Delete a few unit tests that exposed the order in which blocks were analyzed because they printed intermediate results. These tests should be replaced with tests of analyses' public APIs in the future.

kripken · 2023-10-23T20:21:51Z

Would it be difficult to split this up? From the description there are various changes, and reading the code it's not immediately obvious to me which is which.

tlively · 2023-10-23T23:27:47Z

I'll see what I can do.

ashleynh · 2023-10-23T21:50:05Z

test/gtest/cfg.cpp

-  FiniteIntPowersetLattice lattice(numLocals);
-  LivenessTransferFunction transferFunction;
-
-  MonotoneCFGAnalyzer<FiniteIntPowersetLattice, LivenessTransferFunction>


Do you need to replace this test now or will this be future work?

This will be future work. We generally should not have tests that check intermediate results like this because they break when we change internal implementation details. OTOH, the reason the test was like this was that we didn't have a very clean API for accessing the analysis results. I expect to revisit the API problem soon.

(FWIW, generators could help a lot here because we could interleave visiting the expressions and getting their analysis results with test assertions.)

ashleynh · 2023-10-23T21:54:58Z

src/analysis/visitor-transfer-function.h

-      for (auto cfgIter = cfgBlock->begin(); cfgIter != cfgBlock->end();
-           ++cfgIter) {
-        static_cast<SubType*>(this)->visit(*cfgIter);
+      for (auto it = bb.begin(); it != bb.end(); ++it) {


Are you using the .begin() .end() iterator pattern for symmetry with the If-true block above? Just wondering why not the range-based loop that's used on line 263 in src/tools/wasm-fuzz-lattices.cpp?

Yes, it's for symmetry. You're right that a range-based for loop would have worked here as well.

A range-based loop would be better imo, even if it looks more different than the other loop above. I see that as a benefit actually: the difference is more obvious, and separately this one becomes easier to read.

ashleynh · 2023-10-23T22:32:44Z

src/analysis/monotone-analyzer.h

+
+  // The lattice element representing the program state before each block.
+  std::vector<Element> states;
+  // std::vector<BlockState<L>> stateBlocks;


Did you want to delete this commented line?

Yep, thanks 👍

ashleynh · 2023-10-23T23:48:45Z

src/analysis/monotone-analyzer-impl.h

@@ -96,12 +68,21 @@ inline void MonotoneCFGAnalyzer<L, TxFn>::collectResults() {
 template<Lattice L, TransferFunction TxFn>
 inline void MonotoneCFGAnalyzer<L, TxFn>::print(std::ostream& os) {


Can you explain why this print function instead of a << overload? Does it have to do with Concepts?

No particular reason. The original author probably thought print was a nicer interface than <<, and that seems like a reasonable opinion to me. A << overload would have worked just as well. Print methods generally have a slight advantage that they are easier to call from a debugger, although I'm not sure whether it's easy to get the ostream to pass into this one from the debugger. toString methods are generally the easiest for debugging.

tlively · 2023-10-24T21:50:43Z

I don't think it makes sense to split this up after all, since all the changes are either tightly coupled (e.g. removing BlockState and using indices to find block states) or otherwise touch the same code (e.g. replacing the work list data structure and removing the API to initialize the work list). The separable changes are generally variable renamings or changing while loops to for loops, etc, that I don't think would benefit much from being split out.

kripken · 2023-10-24T22:24:29Z

src/analysis/visitor-transfer-function.h

-      for (auto cfgIter = cfgBlock->begin(); cfgIter != cfgBlock->end();
-           ++cfgIter) {
-        static_cast<SubType*>(this)->visit(*cfgIter);
+      for (auto it = bb.begin(); it != bb.end(); ++it) {


A range-based loop would be better imo, even if it looks more different than the other loop above. I see that as a benefit actually: the difference is more obvious, and separately this one becomes easier to read.

tlively · 2023-10-25T17:00:39Z

Merge activity

Oct 25, 1:00 PM: @tlively started a stack merge that includes this pull request via Graphite.
Oct 25, 1:00 PM: @tlively merged this pull request with Graphite.

Simplify the monotone analyzer by replacing all the state it used to store in `BlockState` with a simple vector of lattice elements. Use simple indices to refer to both blocks and their associated states in the vector. Remove the ability for transfer functions to control the initial enqueued order of basic blocks since that was a leaky abstraction. Replace the worklist with a UniqueDeferredQueue since that has generally proven to be more efficient in smiilarly contexts, and more importantly, it has a nicer API. Make miscellaneous simplifications to other code as well. Delete a few unit tests that exposed the order in which blocks were analyzed because they printed intermediate results. These tests should be replaced with tests of analyses' public APIs in the future.

tlively requested review from ashleynh and kripken October 20, 2023 23:08

tlively mentioned this pull request Oct 20, 2023

[analysis][NFC] Create a TransferFunction concept #6033

Merged

Base automatically changed from txfn-concept to main October 21, 2023 00:22

tlively added 2 commits October 20, 2023 17:49

fix bug and restore some tests

dd4546f

tlively force-pushed the simplify-analyzer branch from 734f4cc to dd4546f Compare October 21, 2023 00:49

This was referenced Oct 21, 2023

[analysis][NFC] Rename makeLeastUpperBound to join and move it to lattice #6035

Merged

[analysis] Implement a Bool lattice #6036

Merged

[analysis] Implement an Int lattice #6037

Merged

[analysis] Add a FullLattice concept and Inverted lattice #6038

Merged

missing include

5be5539

This was referenced Oct 21, 2023

[analysis] Implement Flat lattice #6039

Merged

[analysis] Implement a Lift lattice #6040

Merged

ashleynh approved these changes Oct 23, 2023

View reviewed changes

remove stale comment

3804970

kripken approved these changes Oct 24, 2023

View reviewed changes

range-based for loop

409e7ba

tlively mentioned this pull request Oct 25, 2023

[analysis] Improve lattice fuzzer #6050

Merged

tlively merged commit ef8e424 into main Oct 25, 2023
14 checks passed

tlively deleted the simplify-analyzer branch October 25, 2023 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[analysis] Simplify core analysis code #6034

[analysis] Simplify core analysis code #6034

tlively commented Oct 20, 2023

tlively commented Oct 20, 2023 •

edited

Loading

kripken commented Oct 23, 2023

tlively commented Oct 23, 2023

ashleynh Oct 23, 2023

tlively Oct 24, 2023

ashleynh Oct 23, 2023

tlively Oct 24, 2023

kripken Oct 24, 2023

ashleynh Oct 23, 2023

tlively Oct 23, 2023

ashleynh Oct 23, 2023

tlively Oct 23, 2023

tlively commented Oct 24, 2023

kripken Oct 24, 2023

tlively commented Oct 25, 2023 •

edited

Loading

		@@ -96,12 +68,21 @@ inline void MonotoneCFGAnalyzer<L, TxFn>::collectResults() {
		template<Lattice L, TransferFunction TxFn>
		inline void MonotoneCFGAnalyzer<L, TxFn>::print(std::ostream& os) {

[analysis] Simplify core analysis code #6034

[analysis] Simplify core analysis code #6034

Conversation

tlively commented Oct 20, 2023

tlively commented Oct 20, 2023 • edited Loading

kripken commented Oct 23, 2023

tlively commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlively commented Oct 24, 2023

Choose a reason for hiding this comment

tlively commented Oct 25, 2023 • edited Loading

Merge activity

tlively commented Oct 20, 2023 •

edited

Loading

tlively commented Oct 25, 2023 •

edited

Loading