<div class="gmail_quote">On Tue, Jul 10, 2012 at 8:24 PM, Michael Liao <span dir="ltr"><<a href="mailto:michael.liao@intel.com" target="_blank">michael.liao@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
Here is resubmitting of x32 psABI[1] patches against the latest trunk<br>
for code review.</blockquote><div><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">Hello Michael, I wanted to give you some feedback on this feature, and the development process in general. That said, keep in mind that mine is only one voice in the LLVM community. While I've talked to a few others about this, there are still a lot of people who may want to chime in here. They may contradict me or see things differently. But as you've been waiting for some feedback, here is how I see things:</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">
At a fundamental level, looking at LLVM, our developer community and our long term maintainers, I don't think it is in the best interest of the project, the community, or the X86 backend to accept these patches today, and in their current form. They add complexity and a significant long-term maintenance and support burden. They grow the set of supported target platforms, etc.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">At the same time, I think that supporting the x32 platform is important for LLVM long term, and I think you've done a great job of beginning to build out the beginnings of such support.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">So what gives? The primary problem is long-term maintainers. We need active, trusted members of the development community who can shoulder the long term maintenance burden of these patches, the support of this platform, and further generic changes to the x86 backend which are made more challenging or complex due to this additional subtarget. And currently, we don't have any such maintainers.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">I think in order to add this scale of feature to LLVM, your or someone else will need to build a strong reputation within the community, demonstrating that you are willing and able to do the maintenance this requires, and then step forward to support this feature long term. (Alternatively, someone who already has such a reputation might step forward from the community, but I think if anyone were interested and able, they would already have provided code review for you.)</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">
In the hope that you're interested in being that person (and it seems like you are!), what I would suggest is this, in no particular order:</div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">1) Put this up on github as a branch of LLVM. You can maintain it in the open there so that anyone who wants to experiment or try it can. It's possible we could even set up a proper Subversion branch, but that seems to have no real benefit over something like github.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">2) Start working on the existing x86 backend. There are a lot of hacks and kinks in there that could use some serious attention.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">3) Go through the bug tracker, and cherry pick bugs to work on fixing.</div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">4) Join the IRC channel, and participate in the community aspects, answer user questions, join design discussions, etc.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">5) Contribute fixes to the target independent parts of the LLVM backend. This part could use lots of love and attention, its something that any backend will depend on heavily, and can be both very educational and very useful.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">6) Maybe try to build this missing piece that would be a very useful exercise and benefit the community a lot: <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-June/050536.html" target="_blank" class="cremed" style="color:rgb(17,85,204)">http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-June/050536.html</a></div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">7) Once you've really gotten the swing of the development process and have a good rapport with the community, restart the x32 discussion, but start it from a different place: Start a *design* discussion about how best to support x32 in LLVM and Clang.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">8) Get feedback from the community about how to design this, what refactorings would make it cleaner, easier to support, test, etc. Incorporate this feedback, and build consensus and support for the work with the community.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">9) Re-work the code to build on top of the design in #8 and resubmit. At this point, the reviews will likely be both much easier, much more prompt, and frankly much easier for both you and the reviewers. =]</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">I realize this is a *crazy* amount of work "just to add a feature", but without something like this, I think it's much harder for the project to sustain the quality and speed of development it currently enjoys. I also think that at the end of this, LLVM will be better, you'll be in a better position to fix and support users of this feature, and the feature itself will be better implemented.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">
<br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">Now for some very minor comments to help out assuming you follow my rough plan here:</div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">
- Please try to keep things on a single thread. Until the topic changes, when you have an update to a set of patches, just attach them to a new email in the same thread. That makes it much easier to keep track of the discussion.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">- Testing is essential. The current patches have very light to no testing. You need to have thorough regression and/or unit tests for each patch and each component where you're adding support.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">- Incremental development is king. The smaller and more logically focused the patch, the faster it is to code review, and the faster you get to iterate. It's a really hard skill to learn, but thinking in terms of small steps that build on each other to reach your goal is very useful. However, remember that each step should generally be testable, and those tests should be added with that small patch so we don't regress and don't get untested code into the tree.</div>
<div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">
<br></div><div style="color:rgb(34,34,34);background-color:rgb(255,255,255)">I hope this helps, and sorry for the essay. ;] I'm just long-winded... I'm hoping to see more contributions from you!</div><div><span style="background-color:rgb(255,255,255);color:rgb(34,34,34)">-Chandler</span> </div>
</div>