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

[Micah Villmow]

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