Changing JoinFun

[lexpank]

Please put this on hold until I change it on compiler side.

[jstolarek]

Yes, that looks correct. Except for the joinInFacts function, which should also be removed with this patch (as discussed on GHC wiki). Does the package build without warnings?

[lexpank]

Oh, I've completely missed that. Sorry. I'll fix that.

[lexpank]

Changes to GHC are already on Phabricator: https://phabricator.haskell.org/D1808

[mlite]

The label in JoinFun can be useful for the clients. I actually use it
for indexing the live variables of each node.

On 1/20/2016 4:51 AM, Alexander Pankiv wrote:

Please put this on hold until I change it on compiler side.
You can view, comment on, or merge this pull request online at:

#28

-- Commit Summary --

• Changing JoinFun

-- File Changes --

 M src/Compiler/Hoopl/Combinators.hs (6)
 M src/Compiler/Hoopl/Dataflow.hs (8)
 M src/Compiler/Hoopl/Debug.hs (8)
 M src/Compiler/Hoopl/Passes/DList.hs (2)
 M src/Compiler/Hoopl/Passes/Dominator.hs (2)
 M src/Compiler/Hoopl/Pointed.hs (45)
 M src/Compiler/Hoopl/XUtil.hs (12)

-- Patch Links --

https://github.com/haskell/hoopl/pull/28.patch
https://github.com/haskell/hoopl/pull/28.diff

---

Reply to this email directly or view it on GitHub:
#28

[mlite]

Hoop is designed to with the intention to support a wide range of
program analyses, removing the label will make some analyses impossible.

It's a fallacy to use one dataflow analysis example to argue about the
redundancy of some design choices.

On 1/20/2016 4:51 AM, Alexander Pankiv wrote:

Please put this on hold until I change it on compiler side.
You can view, comment on, or merge this pull request online at:

#28

-- Commit Summary --

• Changing JoinFun

-- File Changes --

 M src/Compiler/Hoopl/Combinators.hs (6)
 M src/Compiler/Hoopl/Dataflow.hs (8)
 M src/Compiler/Hoopl/Debug.hs (8)
 M src/Compiler/Hoopl/Passes/DList.hs (2)
 M src/Compiler/Hoopl/Passes/Dominator.hs (2)
 M src/Compiler/Hoopl/Pointed.hs (45)
 M src/Compiler/Hoopl/XUtil.hs (12)

-- Patch Links --

https://github.com/haskell/hoopl/pull/28.patch
https://github.com/haskell/hoopl/pull/28.diff

---

Reply to this email directly or view it on GitHub:
#28

[jstolarek]

I actually use it for indexing the live variables of each node.

Can you give a link to your code where you are doing that? I took a quick look at your hLLVM repo, but all the join functions I was able to find ignored the label argument.

removing the label will make some analyses impossible.
Can you provide a concrete example?

It's a fallacy to use one dataflow analysis example to argue about the redundancy of some design choices.

Our argument (ie. mine and Simon PJ's) was not based on one example. It was based on all examples we have seen.

@AlexanderPankiv - make sure to update the tests as well. At the moment travis build is failing.

[simonpj]

Our argument (ie. mine and Simon PJ's) was not based on one example. It was based on all examples we have seen.
What argument did I make? I’m totally paged out on all this, but is there a wiki page that describes the proposed design change and argues for why it would be a good thing?

Simon

From: Jan Stolarek [mailto:notifications@github.com]
Sent: 21 January 2016 07:59
To: haskell/hoopl hoopl@noreply.github.com
Subject: Re: [hoopl] Changing JoinFun (#28)

I actually use it for indexing the live variables of each node.

Can you give a link to your code where you are doing that? I took a quick look at your hLLVM repo, but all the join functions I was able to find ignored the label argument.

removing the label will make some analyses impossible.
Can you provide a concrete example?

It's a fallacy to use one dataflow analysis example to argue about the redundancy of some design choices.

Our argument (ie. mine and Simon PJ's) was not based on one example. It was based on all examples we have seen.

@AlexanderPankivhttps://github.com/AlexanderPankiv - make sure to update the tests as well. At the moment travis build is failing.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/28#issuecomment-173487992.

[jstolarek]

What argument did I make?

Our argument - or rather, an observation - was that this is never ever used. The intention of Label argument was to be used for debugging purposes but it is a leftover from the times when the library was in development and debugging information was useful to implement it correctly.

is there a wiki page that describes the proposed design change and argues for why it would be a good thing?

Luckily, we wrote it all down: https://ghc.haskell.org/trac/ghc/wiki/Hoopl/Cleanup

Simon, there is one fragment on that wiki page that I don't understand. There is a suggested refactoring from:

analyzeAndRewriteFwd
   :: forall m n f e x entries. (CheckpointMonad m, NonLocal n, LabelsPtr entries)
   => FwdPass m n f
   -> MaybeC e entries   -- THIS ARGUMENT
   -> Graph n e x -> Fact e f
   -> m (Graph n e x, FactBase f, MaybeO x f)

to

analyzeAndRewriteFwd
   :: forall m n f e x entries. (CheckpointMonad m, NonLocal n)
   => FwdPass m n f
   -> Fact e f           -- Entry points plus a fact for each
   -> Graph n e x 
   -> m (Graph n e x, FactBase f, MaybeO x f)

I am completely unable to reconstruct our path of thinking when we wrote this. I see no way of removing information about the entry point of the graph. Are you able to help here?

[simonpj]

I don’t remember either. But looking at it I see

· The old (MaybeC e entries) argument is Nothing in the open case; and is essentially [Label] in the closed case. These are the entry points

· The (Fact e f) argument is a single fact in the open case, and FactBase in the closed case

So I think that in the closed case our intent was to make the domain of the FactBase of the (Fact e f) argument be (by definition) the entry points. The comment says “Entry points plus a fact for each” and that makes perfect sense.

It eliminate a current unstated invariant, that the domain of the incoming (Fact e f) must include all the ‘entries’ argument.

The only things is that we can’t get the domain of a FactBase as a [Label] but only as a [Unique]. I don’t know if that matters. If it does I guess we could change FactBase to make that possible.

Simon

From: Jan Stolarek [mailto:notifications@github.com]
Sent: 21 January 2016 10:20
To: haskell/hoopl hoopl@noreply.github.com
Cc: Simon Peyton Jones simonpj@microsoft.com
Subject: Re: [hoopl] Changing JoinFun (#28)

What argument did I make?

Our argument - or rather, an observation - was that this is never ever used. The intention of Label argument was to be used for debugging purposes but it is a leftover from the times when the library was in development and debugging information was useful to implement it correctly.

is there a wiki page that describes the proposed design change and argues for why it would be a good thing?

Luckily, we wrote it all down: https://ghc.haskell.org/trac/ghc/wiki/Hoopl/Cleanup

Simon, there is one fragment on that wiki page that I don't understand. There is a suggested refactoring from:

analyzeAndRewriteFwd

:: forall m n f e x entries. (CheckpointMonad m, NonLocal n, LabelsPtr entries)

=> FwdPass m n f

-> MaybeC e entries -- THIS ARGUMENT

-> Graph n e x -> Fact e f

-> m (Graph n e x, FactBase f, MaybeO x f)

to

analyzeAndRewriteFwd

:: forall m n f e x entries. (CheckpointMonad m, NonLocal n)

=> FwdPass m n f

-> Fact e f -- Entry points plus a fact for each

-> Graph n e x

-> m (Graph n e x, FactBase f, MaybeO x f)

I am completely unable to reconstruct our path of thinking when we wrote this. I see no way of removing information about the entry point of the graph. Are you able to help here?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/28#issuecomment-173527101.

[jstolarek]

Yes, this line of reasoning does make sense.

The only things is that we can’t get the domain of a FactBase as a [Label] but only as a [Unique]. I don’t know if that matters

Label is a newtype wrapping Unique, so we have all the required information.

[mlite]

On 1/20/2016 11:59 PM, Jan Stolarek wrote:

I actually use it for indexing the live variables of each node.
Can you give a link to your code where you are doing that? I took a quick look at your hLLVM repo, but all the join functions I was able to find ignored the label argument.
The analysis code is not open source code. It's a forward analysis, its
DataLattice is parametrized with a Label->Live Variables mapping that is
computed by a backward analysis (liveness analysis) ahead of time.

This mapping is used to kill the dataflow facts of dead variables. This
optimization might not be necessary for Cmm, but it could be significant
for SSA based IRs like LLVM that has many short living variables.
Without it, I get OOM errors when analyzing some large code. For
example, the LLVM code of pcre2 has some big functions with a lot of SSA
variables.

removing the label will make some analyses impossible.
Can you provide a concrete example?
In general, the label parameter of fact_join provides a way to retrieve
data associated with the current node, the data can be dataflow facts
computed beforehand. For example, It allows using the result of
forward analysis in backward analysis and vice-verse. Maybe the label
was there for the debugging purpose and was not removed before the
release. But it's useful.
It's a fallacy to use one dataflow analysis example to argue about the redundancy of some design choices.
Our argument (ie. mine and Simon PJ's) was not based on one example. It was based on all examples we have seen.
Ok, I stand corrected about the number of examples.

But the proposal to remove fact_bot is questionable. I agree the
proposed revision has less redundant information. But I think it gains
a little convenience at the cost of reducing the readability of complex
code.

Because the dataflow fact (CPFact) used in the argument is simple, the
proposed revision indeed simplifies the code and it does not seem
hamper the readability much. However, for some complex dataflow fact
computations like the above mentioned parametrized dataflow fact
computation, simply being able to specify them as DataLattice make code
easier to understand.

When I first get some code, the first place I start to look at is the
definitions of DataLattice, then the rewriting functions, and the
transfer functions. To me, DataLattice is like a lighthouse guiding
me thru a sea of code. I think other people will find this method
useful as well if (DataLattice, Rewriting function, Transfer function)
consistently exists in all analyses.

@AlexanderPankiv - make sure to update the tests as well. At the moment travis build is failing.

---

Reply to this email directly or view it on GitHub:
#28 (comment)

[jstolarek]

But the proposal to remove fact_bot is questionable. I agree the proposed revision has less redundant information. But I think it gains a little convenience at the cost of reducing the readability of complex code.

The goal of this it to increase readability of code by expressing invariants in the API. Right now you have an API that accepts some arguments that it doesn't use - how does this make the code easier to understand? As I already said, this costed us several days of debugging because we couldn't figure out why the bottom of the lattice is not used. Only after some time we realized that it is not used in any way!

[lexpank]

@jstolarek tests fixed.

[mlite]

On 1/22/2016 7:29 AM, Jan Stolarek wrote:

But the proposal to remove fact_bot is questionable. I agree the proposed revision has less redundant information. But I think it gains a little convenience at the cost of reducing the readability of complex code.
The goal of this it to increase readability of code by expressing invariants in the API. Right now you have an API that accepts some arguments that it doesn't use - how does this make the code easier to understand? As I already said, this costed us several days of debugging because we couldn't figure out why the bottom of the lattice is not used. Only after some time we realized that it is not used in any way!
In your wiki page (https://ghc.haskell.org/trac/ghc/wiki/Hoopl/Cleanup)
example: data CPFact = Bot | CP (Map LocaReg Const)
You said Bot was never used. But this is not equivalent to what I
understand from the above argument: An API that accepts some arguments
that is doesn't use. Dose the API refer to analyzeAndRewriteFwd?

Bot is never used is not equivalant to the last parameter of
analyzeAndRewriteFwd never used?

Do you still have the problematic code around?

BTW, your definition of CPFact is isomorphic to "Map LocalReg Const",
it's quite possible to write code that never uses Bot.

---

Reply to this email directly or view it on GitHub:
#28 (comment)

[mlite]

This change is about removing label in JoinFun. Can we draw a conclusion on this and close it?

We can open an issue to discuss the rest cleanup proposal.

[jstolarek]

An API that accepts some arguments that is doesn't use. Dose the API refer to analyzeAndRewriteFwd?

The API refers to contents of FwdPass and BwdPass data type, so essentially this is about changing analyzeAndRewriteFwd and analyzeAndRewriteBwd.

Bot is never used is not equivalant to the last parameter of analyzeAndRewriteFwd never used?

No. It is equivalent to fact_bot field of fp_lattice field not being used.

Do you still have the problematic code around?

No. In the end we managed to find a way to implement what we wanted without Hoopl.

it's quite possible to write code that never uses Bot.

The important question is: is it possible to write code that ever uses bottom for forward analysis? We believe no.

Can we draw a conclusion on this and close it?

IIUC you want to reject this change?

[mlite]

is it possible to write code that ever uses bottom for forward analysis?

If you supply analyzeAndRewriteFwd with a total function as FactBase, then fact_bot will not be used, otherwise, fact_bot is the default value for any nodes that is not in the domain of FactBase.

Yes, it's possible to write such an analysis. As a matter of fact, Hoopl/testing/ConstProp.hs is such an analysis that uses fact_bot.

IIUC you want to reject this change?

Yes, I reject this change.

[simonpj]

Ning, Jan

There’s a conversation going on here that I do not fully understand.

· I think that Jan and his student are working on Hoopl clean-ups arising from Jan’s internship here at MSR.

· These clean-ups are described here: https://ghc.haskell.org/trac/ghc/wiki/Hoopl/Cleanup

· Generally, as one of the original authors of Hoopl, I’m keen on moving forward on these changes, and I’m delighted to see work happening on them.

· Ning has very helpfully taken on the role of Hoopl maintainer.

· I believe that Ning is not keen on at least some of the proposed changes. He may well have good reasons for this.

Beyond that I have gotten lost in the detail. Can you guys outline what, if any, are the points of disagreement? What are the proposed changes at the moment? Have any of Jan’s proposals being merged? If not, is that because you aren’t happy with them, Ning, or because you haven’t had time?

Thank you both for working on Hoopl – it needs love!

Simon

From: Ning Wang [mailto:notifications@github.com]
Sent: 23 January 2016 15:20
To: haskell/hoopl hoopl@noreply.github.com
Cc: Simon Peyton Jones simonpj@microsoft.com
Subject: Re: [hoopl] Changing JoinFun (#28)

On 1/22/2016 7:29 AM, Jan Stolarek wrote:

But the proposal to remove fact_bot is questionable. I agree the proposed revision has less redundant information. But I think it gains a little convenience at the cost of reducing the readability of complex code.
The goal of this it to increase readability of code by expressing invariants in the API. Right now you have an API that accepts some arguments that it doesn't use - how does this make the code easier to understand? As I already said, this costed us several days of debugging because we couldn't figure out why the bottom of the lattice is not used. Only after some time we realized that it is not used in any way!
In your wiki page (https://ghc.haskell.org/trac/ghc/wiki/Hoopl/Cleanup)
example: data CPFact = Bot | CP (Map LocaReg Const)
You said Bot was never used. But this is not equivalent to what I
understand from the above argument: An API that accepts some arguments
that is doesn't use. Dose the API refer to analyzeAndRewriteFwd?

Bot is never used is not equivalant to the last parameter of
analyzeAndRewriteFwd never used?

Do you still have the problematic code around?

BTW, your definition of CPFact is isomorphic to "Map LocalReg Const",
it's quite possible to write code that never uses Bot.

---

Reply to this email directly or view it on GitHub:
#28 (comment)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/28#issuecomment-174193213.

[jstolarek]

The proposed changes are:

1. Removing Label argument from JoinFun. We have not seen any example that would make any use of that argument. Ning says his closed-source code is making use of that argument. While we haven't seen the code that actually does that, Ning has given a general outline of the idea. I hope Alexander (my student) will be able to construct a proof-of-concept so that we are really sure that this makes sense.
2. Removing bottom from the lattice used in forward analysis. We believed that the bottom element is never used, but Ning points to a code in hoopl testsuite that actually makes use of bottom during forward propagation. Ning is right here - that code really uses bottom in forward propagation. I don't have a good answer here yet why this happens. Perhaps we were wrong arguing that bottom is not required for forward propagation, or perhaps that code contains a logical error that was admitted by the imprecise API. I would have to spend more time thinking about that code in the testsuite, but I have very little time this and next week. Hopefully Alexander can make progress.
3. There was a proposal to introduce consistent coding style, but Ning wants to "minimize changes to the library" and only have changes that "improve usability or add new functionality".

Separately from the discussion here, it is possible that we will have a cleanup of GHC's internal Dataflow module. This is independent of hoopl library.

[simonpj]

OK thanks.

1.  Retaining Label in JoinFun: I’m sure we agree that if it’s useful we should keep it.   Even if not, does it matter much, or is it just a question of cleanliness?
   

2.  Removing bottom: sounds as if there is work to do here.
   

3.  Consistent style. I’m all for it, but people feel differently and sometimes strongly, so I doubt its worth pushing hard on except where there is consensus.
   

So it doesn’t sound as if there is much disagreement – good!

Simon

From: Jan Stolarek [mailto:notifications@github.com]
Sent: 25 January 2016 20:24
To: haskell/hoopl hoopl@noreply.github.com
Cc: Simon Peyton Jones simonpj@microsoft.com
Subject: Re: [hoopl] Changing JoinFun (#28)

The proposed changes are:

1. Removing Label argument from JoinFun. We have not seen any example that would make any use of that argument. Ning says his closed-source code is making use of that argument. While we haven't seen the code that actually does that, Ning has given a general outline of the idea. I hope Alexander (my student) will be able to construct a proof-of-concept so that we are really sure that this makes sense.
2. Removing bottom from the lattice used in forward analysis. We believed that the bottom element is never used, but Ning points to a code in hoopl testsuite that actually makes use of bottom during forward propagation. Ning is right here - that code really uses bottom in forward propagation. I don't have a good answer here yet why this happens. Perhaps we were wrong arguing that bottom is not required for forward propagation, or perhaps that code contains a logical error that was admitted by the imprecise API. I would have to spend more time thinking about that code in the testsuite, but I have very little time this and next week. Hopefully Alexander can make progress.
3. There was a proposal to introduce consistent coding style, but Ning wants to "minimize changes to the library" and only have changes that "improve usability or add new functionality".

Separately from the discussion here, it is possible that we will have a cleanup of GHC's internal Dataflow module. This is independent of hoopl library.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/28#issuecomment-174650743.

[mlite]

· Generally, as one of the original authors of Hoopl, I’m keen on moving forward on these changes, and I’m delighted to see work happening on them.

I'm glad to see Jan's team is working on hoopl. I'm sure it will make hoopl better in the long run.

· Ning has very helpfully taken on the role of Hoopl maintainer. · I believe that Ning is not keen on at least some of the proposed changes. He may well have good reasons for this.

Thanks for the trust. I might be too conservative. It's always good to have other stakeholders (Jan's team and ghc-dev) to pull it in the another direction.

Beyond that I have gotten lost in the detail. Can you guys outline what, if any, are the points of disagreement? What are the proposed changes at the moment? Have any of Jan’s proposals being merged? If not, is that because you aren’t happy with them, Ning, or because you haven’t had time?

Jan summarized the recent conversation well. I want to answer the question about Jan's students' pull requests.

• The pull request to remove the label argument of JoinFun. I intended to reject it. The label argument might be initially introduced for debugging, but it was not removed before the release. Once a feature is released to the public, it might be used. Actually, the label argument makes stacking up the pre-computed data flow facts possible, it is quite nice. I don't like removing it.
• Michal's pull requests to remove CheckpointMonad, and to enforce coding style. I don't have any objections, but they are not merged yet.

[lexpank]

Hello. I'm not trying to be rude, but I have question (especilally to @mlite ):

I've been trying to check if fact_bot is really used if forward analysis, as discussed:

Yes, it's possible to write such an analysis. As a matter of fact, Hoopl/testing/ConstProp.hs is such an analysis that uses fact_bot.

And changed getFact function in src/Compiler/Hoopl/Dataflow.hs to:

getFact  :: DataflowLattice f -> Label -> FactBase f -> f
getFact lat l fb = case lookupFact l fb of Just  f -> f
                                           Nothing -> undefined

I know it doesn't make much sense (because we need fact_bot for backward analysis), but the thing is: after running tests - all passed. In other words, it seems to me that bottom isn't used in case of tests. Can anyone tell me if I'm doing/getting something wrong?

I appreciate your help.

[michalt]

So this thread is starting to grow really large and discuss too many things at the same time. Initially it was about removing the Label parameter, so maybe let's keep this focused on that change (and discuss whether we need bottom to a separate PR or issue). Hope that's ok with everybody.

As for removing the Label param, I didn't have time to look into this yet, I'll try to find some time later this week.

[mlite]

AlexanderPankiv, please open another issue to discuss the remaining cleanup proposal.
I cannot test your change now. But you can also set fact_bot of ConstProp as undefined and see what happens.

[lexpank]

should be reworked anyway