Making an API for doing flow analysis

[gabrielsferre]

Abstracting away common patterns of the flow analysis code for reusability.

[hugomg]

In my first look at the code, I don't understand whats the deal with FlowState.Build and FlowState.Apply. The FlowState.Build looks familiar but I don't get what Apply is all about. Can you help me understand that? I think I need that to be able to complete the review.

[gabrielsferre]

The "flow.flow_analysis" function returns only the "start" and "finish" sets for each block ("start" and "finish" might mean "input" and "output" or vice versa, depending on the order the flow analysis is being done), so we still have to determine what is the set of values for each command. The way to do it is using this FlowState.Apply thing, where you pass an object of type FlowState.Apply to the flow.update_set function and it updates the set for you. The reason I needed this two kinds of flow states (FlowState.Apply and FlowState.Build) is that I had to know whether I was updating the kill/gen sets or the actual set of values that is returned to the user.

Currently, the user of the API has to update the FlowState.Apply states manually when looping through commands, which to me feels kinda confusing but is the only way I could think of making it work if we didn't want to save a set for each command.

Is it ok to save a set for each command? If it is then this part of the API could be much simpler. We'd be able to do all the necessary processing inside flow.flow_analysis. We wouldn't be using all those sets inside the "loop until things converge" part of the flow analysis algorithm, only at the end, after we already have the correct input/output sets.

[hugomg]

After thinking about it, I think the main problems with the FlowState is that the process_cmd callback is not a pure function. It calls kill_value() and does different things depending on whether we are in BUILD or APPLY mode.

One alternative that would be better, but still not my favorite, would be to turn gen_value and kill_value into callbacks passed to process_cmd. This way, we can use different callbacks when we're in BUILD and when we're in APPLY mode.

But I think the best way would be to turn process_cmd into a pure function. It could receive a single argument (the IR command) and return two tables (GEN and KILL). And while we're at it, rename it into "compute_gen_kill".
During the build phase we can combine the individual GEN/KILL into a GEN/KILL for the entire basic block. And during apply phase we can use them to compute the resulting sets for each command.

I think it's ok if we call compute_gen_kill twice, once during build phase and once during apply phase. But caching the results is also OK. Focus on keeping the code as simple and easy to understand. We can worry about optimizations later.

[gabrielsferre]

Ok, I'll try doing the pure version of process_cmd.

[hugomg]

OK, please re-request the review when you're ready.

[hugomg]

Tá com uma cara boa, mas acho que a disposição dos comentários está desequilibrada. Tem um comentário grande demais no topo e zero comentários no resto. O melhor lugar pra dizer o que cada função recebe e retorna é do lado da própria função. (Isso é especialmente importante pra saber o que a função retorna, que nem sempre dá pra adivinhar pelo nome)