llvm dev - Nov 2012 - [LLVMdev] Heads up, I've backed out significant amounts of the multiple address space conversion changes

TL;DR: See subject. That is all.
----

First off, I want to say I'm sorry for doing this. =/

There were several issues that led to this:

1) There were several micro-design problems with the APIs as
implemented in the core libraries of LLVM. These then fanned out to
all the uses of these libraries making fixing these issues challenging
as well. The fixes were also not happening in a timely fashion, and
instead a lot of code was switched over to the not-yet-right
interfaces.

2) The updates to the broader libraries using the pointer types and
address spaces were not really well factored to use all of the helper
and convenience routines available, and add ones where missing. Some
helpers were added, but not always enough, and not always in time for
some of the usages to reflect these helpers.

3) The updates to the broader libraires, especially the transforms and
analysis libraries were never accompanied with test cases for those
transforms or analyses to even sanity check the changes. A few of
these had bugs spotted by inspection during post-commit review, but we
don't even really know how many more might be lurking.

4) The commits were poorly factored into isolated, incremental chunks.
Notably, reworking of core APIs was mixed with updating their
consumers, and with general cleanups. This makes isolated reverts
impossible, and general reverts increasingly hard as time passes.

5) There is at least one serious bug with the methodology being
employed in some of the usage patterns, leading to PR14233.

6) Micah is out of the office for a while and not likely to have the
time necessary to address these issues quickly.

7) We're going to be branching for 3.2 in not-too-long.


When you combine all these factors, the best option I saw was to back
out the widespread changes outside of the core libraries, and some of
the more problematic changes to the core libraries, leaving as much of
the work on DataLayout etc in tree as possible. This fixes PR14233, my
immediate need as this is a serious regression, and it paves the way
for a relatively safe state to release 3.2 in even if Micah doesn't
get significant time to work on this prior to the release branching.

I didn't make this decision in isolation though. I talked extensively
about these issues with Nick Lewycky, Richard Smith, Benjamin Kramer,
and Duncan Sands. Duncan and I made the final call to pull these
patches out once I heard that Micah was going to be out and unable te
quickly address an PRs, etc.

Since I made the call to pull it out, I did the work to disentangle
the commits, and have backed it out in a series of commits tonight.
I'll plan on doing some of the cleanups and API fixes in the core
libraries as well to ensure those are in place prior to 3.2.


Some things I think are going to be pretty important when you try to
return to this Micah in order to have a bit more stability and success
getting this into the tree and staying there:

- I think we should spend some time iterating on the core APIs used to
query, test, set and manipulate the address space and pointer types
with address spaces. Until these interfaces look really good, you
shouldn't convert many users.

- Any conversion changes that just switch usage patterns should be
separate commits from any changes to the APIs being used. Any generic
cleanups that remove even the need for a conversion change should also
be in their own independent commit.

- You should keep a running patch that converts a few core passes and
bits of LLVM to use the new address space APIs, and use that to figure
out the issues with the core APIs.

- Each of the changes to a core API like DataLayout should (where
possible) be tested directly via a unittest.

- Each of the conversions for a pass or target should be accompanied
by test cases with multiple address spaces so that other developers
don't blindly break the support constructs implemented.


Anyways, happy to talk more about how to go about making these
sweeping changes when you're back Micah.

On Nov 1, 2012, at 2:56 AM, Chandler Carruth <chandlerc at gmail.com>
wrote:
> TL;DR: See subject. That is all.
> ----
> 
> First off, I want to say I'm sorry for doing this. =/
It's a hard call, but thank you for doing this.  I fully agree that it was
the right thing to do, it doesn't make sense to force a major change like
this in right before the 3.2 branch date.

-Chris

> 
> There were several issues that led to this:
> 
> 1) There were several micro-design problems with the APIs as
> implemented in the core libraries of LLVM. These then fanned out to
> all the uses of these libraries making fixing these issues challenging
> as well. The fixes were also not happening in a timely fashion, and
> instead a lot of code was switched over to the not-yet-right
> interfaces.
> 
> 2) The updates to the broader libraries using the pointer types and
> address spaces were not really well factored to use all of the helper
> and convenience routines available, and add ones where missing. Some
> helpers were added, but not always enough, and not always in time for
> some of the usages to reflect these helpers.
> 
> 3) The updates to the broader libraires, especially the transforms and
> analysis libraries were never accompanied with test cases for those
> transforms or analyses to even sanity check the changes. A few of
> these had bugs spotted by inspection during post-commit review, but we
> don't even really know how many more might be lurking.
> 
> 4) The commits were poorly factored into isolated, incremental chunks.
> Notably, reworking of core APIs was mixed with updating their
> consumers, and with general cleanups. This makes isolated reverts
> impossible, and general reverts increasingly hard as time passes.
> 
> 5) There is at least one serious bug with the methodology being
> employed in some of the usage patterns, leading to PR14233.
> 
> 6) Micah is out of the office for a while and not likely to have the
> time necessary to address these issues quickly.
> 
> 7) We're going to be branching for 3.2 in not-too-long.
> 
> 
> When you combine all these factors, the best option I saw was to back
> out the widespread changes outside of the core libraries, and some of
> the more problematic changes to the core libraries, leaving as much of
> the work on DataLayout etc in tree as possible. This fixes PR14233, my
> immediate need as this is a serious regression, and it paves the way
> for a relatively safe state to release 3.2 in even if Micah doesn't
> get significant time to work on this prior to the release branching.
> 
> I didn't make this decision in isolation though. I talked extensively
> about these issues with Nick Lewycky, Richard Smith, Benjamin Kramer,
> and Duncan Sands. Duncan and I made the final call to pull these
> patches out once I heard that Micah was going to be out and unable te
> quickly address an PRs, etc.
> 
> Since I made the call to pull it out, I did the work to disentangle
> the commits, and have backed it out in a series of commits tonight.
> I'll plan on doing some of the cleanups and API fixes in the core
> libraries as well to ensure those are in place prior to 3.2.
> 
> 
> Some things I think are going to be pretty important when you try to
> return to this Micah in order to have a bit more stability and success
> getting this into the tree and staying there:
> 
> - I think we should spend some time iterating on the core APIs used to
> query, test, set and manipulate the address space and pointer types
> with address spaces. Until these interfaces look really good, you
> shouldn't convert many users.
> 
> - Any conversion changes that just switch usage patterns should be
> separate commits from any changes to the APIs being used. Any generic
> cleanups that remove even the need for a conversion change should also
> be in their own independent commit.
> 
> - You should keep a running patch that converts a few core passes and
> bits of LLVM to use the new address space APIs, and use that to figure
> out the issues with the core APIs.
> 
> - Each of the changes to a core API like DataLayout should (where
> possible) be tested directly via a unittest.
> 
> - Each of the conversions for a pass or target should be accompanied
> by test cases with multiple address spaces so that other developers
> don't blindly break the support constructs implemented.
> 
> 
> Anyways, happy to talk more about how to go about making these
> sweeping changes when you're back Micah.
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

First off I want to thank everyone for helping out with this. While I don't
like that the code was backed out, I would do the same thing in your shoes given
the same situation.

So, what should the next step be? I'm still catching up on what exactly is
still there and what was removed, but that shouldn't stop the discussion
from continuing.

Micah 
> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu]
> On Behalf Of Chandler Carruth
> Sent: Thursday, November 01, 2012 2:56 AM
> To: LLVM Developers Mailing List; Micah Villmow; Duncan Sands
> Subject: [LLVMdev] Heads up, I've backed out significant amounts of the
> multiple address space conversion changes
> 
> TL;DR: See subject. That is all.
> ----
> 
> First off, I want to say I'm sorry for doing this. =/
> 
> There were several issues that led to this:
> 
> 1) There were several micro-design problems with the APIs as
> implemented in the core libraries of LLVM. These then fanned out to
> all the uses of these libraries making fixing these issues challenging
> as well. The fixes were also not happening in a timely fashion, and
> instead a lot of code was switched over to the not-yet-right
> interfaces.
> 
> 2) The updates to the broader libraries using the pointer types and
> address spaces were not really well factored to use all of the helper
> and convenience routines available, and add ones where missing. Some
> helpers were added, but not always enough, and not always in time for
> some of the usages to reflect these helpers.
> 
> 3) The updates to the broader libraires, especially the transforms and
> analysis libraries were never accompanied with test cases for those
> transforms or analyses to even sanity check the changes. A few of
> these had bugs spotted by inspection during post-commit review, but we
> don't even really know how many more might be lurking.
> 
> 4) The commits were poorly factored into isolated, incremental chunks.
> Notably, reworking of core APIs was mixed with updating their
> consumers, and with general cleanups. This makes isolated reverts
> impossible, and general reverts increasingly hard as time passes.
> 
> 5) There is at least one serious bug with the methodology being
> employed in some of the usage patterns, leading to PR14233.
> 
> 6) Micah is out of the office for a while and not likely to have the
> time necessary to address these issues quickly.
> 
> 7) We're going to be branching for 3.2 in not-too-long.
> 
> 
> When you combine all these factors, the best option I saw was to back
> out the widespread changes outside of the core libraries, and some of
> the more problematic changes to the core libraries, leaving as much of
> the work on DataLayout etc in tree as possible. This fixes PR14233, my
> immediate need as this is a serious regression, and it paves the way
> for a relatively safe state to release 3.2 in even if Micah doesn't
> get significant time to work on this prior to the release branching.
> 
> I didn't make this decision in isolation though. I talked extensively
> about these issues with Nick Lewycky, Richard Smith, Benjamin Kramer,
> and Duncan Sands. Duncan and I made the final call to pull these
> patches out once I heard that Micah was going to be out and unable te
> quickly address an PRs, etc.
> 
> Since I made the call to pull it out, I did the work to disentangle
> the commits, and have backed it out in a series of commits tonight.
> I'll plan on doing some of the cleanups and API fixes in the core
> libraries as well to ensure those are in place prior to 3.2.
> 
> 
> Some things I think are going to be pretty important when you try to
> return to this Micah in order to have a bit more stability and success
> getting this into the tree and staying there:
> 
> - I think we should spend some time iterating on the core APIs used to
> query, test, set and manipulate the address space and pointer types
> with address spaces. Until these interfaces look really good, you
> shouldn't convert many users.
> 
> - Any conversion changes that just switch usage patterns should be
> separate commits from any changes to the APIs being used. Any generic
> cleanups that remove even the need for a conversion change should also
> be in their own independent commit.
> 
> - You should keep a running patch that converts a few core passes and
> bits of LLVM to use the new address space APIs, and use that to figure
> out the issues with the core APIs.
> 
> - Each of the changes to a core API like DataLayout should (where
> possible) be tested directly via a unittest.
> 
> - Each of the conversions for a pass or target should be accompanied
> by test cases with multiple address spaces so that other developers
> don't blindly break the support constructs implemented.
> 
> 
> Anyways, happy to talk more about how to go about making these
> sweeping changes when you're back Micah.
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

I guess I'll try picking this back up. What were the problems exactly from a
design perspective?

On Nov 1, 2012, at 2:56 , Chandler Carruth <chandlerc at gmail.com> wrote:
> TL;DR: See subject. That is all.
> ----
> 
> First off, I want to say I'm sorry for doing this. =/
> 
> There were several issues that led to this:
> 
> 1) There were several micro-design problems with the APIs as
> implemented in the core libraries of LLVM. These then fanned out to
> all the uses of these libraries making fixing these issues challenging
> as well. The fixes were also not happening in a timely fashion, and
> instead a lot of code was switched over to the not-yet-right
> interfaces.
> 
> 2) The updates to the broader libraries using the pointer types and
> address spaces were not really well factored to use all of the helper
> and convenience routines available, and add ones where missing. Some
> helpers were added, but not always enough, and not always in time for
> some of the usages to reflect these helpers.
> 
> 3) The updates to the broader libraires, especially the transforms and
> analysis libraries were never accompanied with test cases for those
> transforms or analyses to even sanity check the changes. A few of
> these had bugs spotted by inspection during post-commit review, but we
> don't even really know how many more might be lurking.
> 
> 4) The commits were poorly factored into isolated, incremental chunks.
> Notably, reworking of core APIs was mixed with updating their
> consumers, and with general cleanups. This makes isolated reverts
> impossible, and general reverts increasingly hard as time passes.
> 
> 5) There is at least one serious bug with the methodology being
> employed in some of the usage patterns, leading to PR14233.
> 
> 6) Micah is out of the office for a while and not likely to have the
> time necessary to address these issues quickly.
> 
> 7) We're going to be branching for 3.2 in not-too-long.
> 
> 
> When you combine all these factors, the best option I saw was to back
> out the widespread changes outside of the core libraries, and some of
> the more problematic changes to the core libraries, leaving as much of
> the work on DataLayout etc in tree as possible. This fixes PR14233, my
> immediate need as this is a serious regression, and it paves the way
> for a relatively safe state to release 3.2 in even if Micah doesn't
> get significant time to work on this prior to the release branching.
> 
> I didn't make this decision in isolation though. I talked extensively
> about these issues with Nick Lewycky, Richard Smith, Benjamin Kramer,
> and Duncan Sands. Duncan and I made the final call to pull these
> patches out once I heard that Micah was going to be out and unable te
> quickly address an PRs, etc.
> 
> Since I made the call to pull it out, I did the work to disentangle
> the commits, and have backed it out in a series of commits tonight.
> I'll plan on doing some of the cleanups and API fixes in the core
> libraries as well to ensure those are in place prior to 3.2.
> 
> 
> Some things I think are going to be pretty important when you try to
> return to this Micah in order to have a bit more stability and success
> getting this into the tree and staying there:
> 
> - I think we should spend some time iterating on the core APIs used to
> query, test, set and manipulate the address space and pointer types
> with address spaces. Until these interfaces look really good, you
> shouldn't convert many users.
> 
> - Any conversion changes that just switch usage patterns should be
> separate commits from any changes to the APIs being used. Any generic
> cleanups that remove even the need for a conversion change should also
> be in their own independent commit.
> 
> - You should keep a running patch that converts a few core passes and
> bits of LLVM to use the new address space APIs, and use that to figure
> out the issues with the core APIs.
> 
> - Each of the changes to a core API like DataLayout should (where
> possible) be tested directly via a unittest.
> 
> - Each of the conversions for a pass or target should be accompanied
> by test cases with multiple address spaces so that other developers
> don't blindly break the support constructs implemented.
> 
> 
> Anyways, happy to talk more about how to go about making these
> sweeping changes when you're back Micah.
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Outside of what was listed below, which you would have to go back into the other
emails/reviews to get into more details, I believe the handling of global
constants expressions was problematic with the API's that I had implemented.

That said, changes of this magnitude should be done in a branch instead of
mainline trunk.

Micah

-----Original Message-----
From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On
Behalf Of Matt Arsenault
Sent: Thursday, June 27, 2013 11:39 AM
To: Chandler Carruth
Cc: Micah Villmow; LLVM Developers Mailing List
Subject: Re: [LLVMdev] Heads up, I've backed out significant amounts of the
multiple address space conversion changes

I guess I'll try picking this back up. What were the problems exactly from a
design perspective?

On Nov 1, 2012, at 2:56 , Chandler Carruth <chandlerc at gmail.com> wrote:
> TL;DR: See subject. That is all.
> ----
> 
> First off, I want to say I'm sorry for doing this. =/
> 
> There were several issues that led to this:
> 
> 1) There were several micro-design problems with the APIs as 
> implemented in the core libraries of LLVM. These then fanned out to 
> all the uses of these libraries making fixing these issues challenging 
> as well. The fixes were also not happening in a timely fashion, and 
> instead a lot of code was switched over to the not-yet-right 
> interfaces.
> 
> 2) The updates to the broader libraries using the pointer types and 
> address spaces were not really well factored to use all of the helper 
> and convenience routines available, and add ones where missing. Some 
> helpers were added, but not always enough, and not always in time for 
> some of the usages to reflect these helpers.
> 
> 3) The updates to the broader libraires, especially the transforms and 
> analysis libraries were never accompanied with test cases for those 
> transforms or analyses to even sanity check the changes. A few of 
> these had bugs spotted by inspection during post-commit review, but we 
> don't even really know how many more might be lurking.
> 
> 4) The commits were poorly factored into isolated, incremental chunks.
> Notably, reworking of core APIs was mixed with updating their 
> consumers, and with general cleanups. This makes isolated reverts 
> impossible, and general reverts increasingly hard as time passes.
> 
> 5) There is at least one serious bug with the methodology being 
> employed in some of the usage patterns, leading to PR14233.
> 
> 6) Micah is out of the office for a while and not likely to have the 
> time necessary to address these issues quickly.
> 
> 7) We're going to be branching for 3.2 in not-too-long.
> 
> 
> When you combine all these factors, the best option I saw was to back 
> out the widespread changes outside of the core libraries, and some of 
> the more problematic changes to the core libraries, leaving as much of 
> the work on DataLayout etc in tree as possible. This fixes PR14233, my 
> immediate need as this is a serious regression, and it paves the way 
> for a relatively safe state to release 3.2 in even if Micah doesn't 
> get significant time to work on this prior to the release branching.
> 
> I didn't make this decision in isolation though. I talked extensively 
> about these issues with Nick Lewycky, Richard Smith, Benjamin Kramer, 
> and Duncan Sands. Duncan and I made the final call to pull these 
> patches out once I heard that Micah was going to be out and unable te 
> quickly address an PRs, etc.
> 
> Since I made the call to pull it out, I did the work to disentangle 
> the commits, and have backed it out in a series of commits tonight.
> I'll plan on doing some of the cleanups and API fixes in the core 
> libraries as well to ensure those are in place prior to 3.2.
> 
> 
> Some things I think are going to be pretty important when you try to 
> return to this Micah in order to have a bit more stability and success 
> getting this into the tree and staying there:
> 
> - I think we should spend some time iterating on the core APIs used to 
> query, test, set and manipulate the address space and pointer types 
> with address spaces. Until these interfaces look really good, you 
> shouldn't convert many users.
> 
> - Any conversion changes that just switch usage patterns should be 
> separate commits from any changes to the APIs being used. Any generic 
> cleanups that remove even the need for a conversion change should also 
> be in their own independent commit.
> 
> - You should keep a running patch that converts a few core passes and 
> bits of LLVM to use the new address space APIs, and use that to figure 
> out the issues with the core APIs.
> 
> - Each of the changes to a core API like DataLayout should (where
> possible) be tested directly via a unittest.
> 
> - Each of the conversions for a pass or target should be accompanied 
> by test cases with multiple address spaces so that other developers 
> don't blindly break the support constructs implemented.
> 
> 
> Anyways, happy to talk more about how to go about making these 
> sweeping changes when you're back Micah.
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
LLVM Developers mailing list
LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev