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

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

On Thu, Jun 27, 2013 at 12:49 PM, Micah Villmow <micah.villmow at
smachines.com
> wrote:
> That said, changes of this magnitude should be done in a branch instead of
> mainline trunk.

I strongly disagree. If you think this is the case, we should probably
start a new thread (rather than ressurecting this one) with the context of
what you want to do and why you think it should be on a branch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130627/313e4a7d/attachment.html>

The reason why I say it should be done in a separate branch is that the final
design is not necessarily the same as the first initial implementation. There
are things that will break and this kind of change touches almost everything,
not just the core LLVM libraries. It is hard to get right the first time, and
developing it in a sandbox will dramatically decrease the amount of churn that
will show up in all the various components on mainline.

Just from my experience, the following items have to be solved or considered
that my first implementation didn’t handle. Now I am not doing this work, and I
haven’t looked at this in many months, so this is based on my memory, so I might
be off.


·         Global constant expression handling API’s assume pointer size of
address space zero, what about constants in different address spaces?

·         Alloca always assumes address space zero pointer sizes, how does it
work for different address spaces?

·         How to test everything without exponentially increasing the number of
tests?

That is in addition to the large amount of churn to not only the
internal/external LLVM API’s, but also its components(clang, lldb, etc…).

Now if you break everything into tiny patches, it might be manageable on
mainline, but even changing one API can cascade to tens of files.

Micah
From: chandlerc at google.com [mailto:chandlerc at google.com] On Behalf Of
Chandler Carruth
Sent: Thursday, June 27, 2013 4:18 PM
To: Micah Villmow
Cc: Matt Arsenault; Chandler Carruth; LLVM Developers Mailing List
Subject: Re: [LLVMdev] Heads up, I've backed out significant amounts of the
multiple address space conversion changes


On Thu, Jun 27, 2013 at 12:49 PM, Micah Villmow <micah.villmow at
smachines.com<mailto:micah.villmow at smachines.com>> wrote:
That said, changes of this magnitude should be done in a branch instead of
mainline trunk.

I strongly disagree. If you think this is the case, we should probably start a
new thread (rather than ressurecting this one) with the context of what you want
to do and why you think it should be on a branch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130627/99c3608d/attachment.html>