[PATCH] D20260: IR: Introduce local_unnamed_addr attribute.

Wed May 18 10:14:57 PDT 2016

On Wed, May 18, 2016 at 10:04 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On May 18, 2016, at 9:53 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, May 18, 2016 at 8:22 AM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>> Hi David,
>>
>>
>> On May 17, 2016, at 11:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Tue, May 17, 2016 at 11:40 PM, Mehdi Amini <mehdi.amini at apple.com>
>> wrote:
>>
>>>
>>> On May 17, 2016, at 11:33 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> On Tue, May 17, 2016 at 11:24 PM, Mehdi Amini <mehdi.amini at apple.com>
>>> wrote:
>>>
>>>>
>>>> On May 17, 2016, at 11:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, May 17, 2016 at 9:58 PM, Mehdi Amini via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>>
>>>>> > On May 17, 2016, at 9:48 PM, Justin Bogner <mail at justinbogner.com>
>>>>> wrote:
>>>>> >
>>>>> > Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org> writes:
>>>>> >>    On May 17, 2016, at 5:21 PM, Chandler Carruth <
>>>>> chandlerc at gmail.com> wrote:
>>>>> >>
>>>>> >>    On Tue, May 17, 2016 at 5:08 PM Mehdi Amini via llvm-commits <
>>>>> >>    llvm-commits at lists.llvm.org> wrote:
>>>>> >>
>>>>> >>        Seriously I absolutely don't get the "start a new revision"
>>>>> thing
>>>>> >>        because the initial email was not on llvm-commit. Really
>>>>> what is the
>>>>> >>        rational?
>>>>> >>
>>>>> >>        We are losing the history and the threading for the review,
>>>>> this is
>>>>> >>        really annoying.
>>>>> >>
>>>>> >>        So big -1
>>>>> >>
>>>>> >>    I don't care very much about starting a new revision.
>>>>> >>
>>>>> >>    Justin and others who don't use phabricator asked for this
>>>>> because that is
>>>>> >>    what causes an email with a patch file to be sent to
>>>>> llvm-commits, and so
>>>>> >>    I've been trying to encourage it based on their request.
>>>>> >>
>>>>> >> There *has been* an email with a patch sent to llvm-commit after
>>>>> the first
>>>>> >> update to the diff:
>>>>> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160516/357163.html
>>>>> >> Just "having a patch sent to llvm-commit" can be accomplished with
>>>>> an manual
>>>>> >> email answering to the revision instead of create a new one from
>>>>> scratch.
>>>>> >
>>>>> > Yes, there's a patch, and it explains what's happening as "Minimise
>>>>> the
>>>>> > diff a little" and "Add assembler/bitcode test". The description of
>>>>> > what's happening is completely missing. The comments so far aren't on
>>>>> > the mailing list at all
>>>>>
>>>>> Re-creating a revision is not gonna help to put this history back on
>>>>> the mailing list.
>>>>>
>>>>>
>>>>> > , and somebody who reads this mail first has no
>>>>> > idea what's going on. Is the patch something interesting? Should I
>>>>> spend
>>>>> > my time following the link and catching up? It also has a subject
>>>>> that
>>>>> > starts with "Re:", which makes me and my email filter think I've
>>>>> already
>>>>> > read the first patch and decided whether or not to pay attention.
>>>>> >
>>>>> > Essentially, patches that start without llvm commits don't look like
>>>>> > mails that need to be read.
>>>>>
>>>>> I judge based on the title, and then click on the link.
>>>>>
>>>>> >
>>>>> > The first email in a patch review thread is incredibly important. It
>>>>> > summarizes the point of the patch, provides context, and has the
>>>>> patch.
>>>>> > Coming into the conversation half way through is very difficult.
>>>>>
>>>>> We all agree that it is *better* to have llvm-commit as an initial
>>>>> subscriber, but unless someone put the manpower in to get it enforced by
>>>>> phabricator, we'll have to live with the fact that occasionally it will be
>>>>> missing.
>>>>>
>>>>
>>>> I'm pretty sure the warning that turns up in Phab suggests creating a
>>>> new code review with *-commits on from the start. I'd encourage this to be
>>>> the path we encourage/request when we see this (for myself, it's really
>>>> obvious when someone creates a review with me as a reviewer but without the
>>>> mailing list - such a mail ends up in my inbox, whereas if it had the
>>>> mailing list on it it'd be in my mailing list bucket). I think we should do
>>>> it early and often. Yes, fixing Phab would be better. But I think
>>>> restarting the review is better than continuing it without the
>>>> original/initial context - earlier the better, before lots of conversation
>>>> has already happened off list.
>>>>
>>>>
>>>> That's the point: *after* some round of reviews were already conducted
>>>> on Phabricator, the most important context for the review *is* now this
>>>> review history and this is what is important to preserve, and this is why
>>>> restarting a new revision in this condition is a non-starter to me.
>>>>
>>>
>>> Except none of that is on the mailing list archive or otherwise
>>> preserved in LLVM's system of record. It was pretty well discussed when
>>> Phab was introduced that the mailing list would be the system of record and
>>> Phab just a tool. If it's not on the list, it's not here.
>>>
>>>
>>> Except it exists, and in the present time it is useful
>>> context/information, so "it's not here" is definitely not an accurate
>>> representation of the situation.
>>>
>>>
>>> In the case where a bunch of review without the list - we should
>>> reintroduce any of that context/discussion.
>>>
>>>
>>> As said before, opening a new review is not gonna make this appear
>>> magically.
>>>
>>
>> I'm not suggesting it appear by magic. I'm suggesting that the person
>> (Peter) who creates the new review, summarize the existing off-list (&
>> partly on list as well) discussion in that new review, so everyone's got
>> the context to engage in this review. (& so it's recorded on the mailing
>> list for archival purposes, etc)
>>
>>
>> But why does it have to be in a *new* review? How forwarding the original
>> email that started the thread not enough to get the initial context?
>>
>>
>> I don't know whether a new thread's especially necessary, but at least an
>>> authoritative email with all that context being summarized into this thread
>>> is called for. It'll be harder to follow the thread (people expect the
>>> first email to have the introductory context, etc) but not impossible.
>>>
>>>
>>> I'd probably favor a new thread, with the old one referenced for
>>> archaeology - the current state can be summarized, any outstanding comments
>>> can be replied to in the new thread with any context quoted as needed.
>>>
>>>
>>> If I spent time reviewing a revision, using inline comment on
>>> Phabricator, and a further round of review is done with a different
>>> revision, I don't see my inline comments and I can't figure if they were
>>> addressed, and I can't really see what changed between the two revisions.
>>>
>>
>> That just brings us back to where we would be if we didn't have Phab -
>> that's what we all had to do anyway, read the patch, read someone's
>> unstructured comments, etc. And other people in the community are seeing a
>> worse situation - because they're not using/looking at Phab they're missing
>> all that context .
>>
>>
>>
>>> This, as a reviewer, is most important than anything else for me, and I
>>> consider that opening a new revision in this condition is a waste of *my*
>>> time as a reviewer, and this is for benefit that sounds hand-wavy to me. So
>>> in the future I'll probably resign as a reviewer on the any new revision
>>> created in this condition.
>>>
>>
>> This last bit seems like it's entirely the point - if you notice a review
>> created in this condition if the first thing you do is say "this isn't in a
>> good condition, please create one with the mailing list on it, I'll review
>> that one" - then we get exactly what we all seem to want, right?
>>
>>
>> You didn't get my last bit the way I meant it apparently, let me try
>> again:
>>
>> 1) A revision is created without llvm-commits as a subscriber
>> 2) I put inline comments in the review and we go through one or multiple
>> round of reviews.
>> 3) At some point during the process (before 2 or between two rounds of
>> review, llvm-commits gets added)
>> 4) People complains because there didn't get the previous emails and ask
>> to start a new revision
>>
>> Of course if we catch it during 1, we can still act on it (forward the
>> email to llvm-commits, or nuke the revision and start a new one, same
>> effect to me), and it seems to me that it is what you're saying, right?
>> However, the case I am complaining about here, is when we don't catch it
>> initially and we end up in 4. What do we do now? Restarting a new revision
>> nukes the review history and all my inline comments are lost, i.e. I'm not
>> OK to restart on a new revision.
>>
>
> OK, thanks for clarifying. Why would this be unacceptable to you?
>
>
> It is very annoying to me because I rely heavily on the fact that I'll be
> able to see the new revision with my inline comments to see what was
> updated compared to what I commented. Or what files are now changed.
> The ability to see the change between arbitrary revision in Phabricator is
> also broken when you open a new revision.
>
>
> This is how a non-Phab review would progress, right? (unstructured replies
>
>
> Yes, which is also why I'm not much involved in these (quickly looking in
> 2016 I was involved in ~200 reviews, and it seems at most 3 or 4 we not on
> Phab). I.e. we wouldn't have Phab, I would review less.
>
>
>
> - usually in infix email style, responding to your comments - the same
> thing could be fairly easily replicated in an email response (by the
> original author - quoting your off list review emails and interspersing
> their reply, etc) to the new thread)
>
>
>> So while it is unfortunate that it was not caught earlier, I argue that
>> the inconvenience for the reviewer should prevail on the inconvenience for
>> the hypothetical people that may engage in the review: if sending them the
>> original email is not enough, either they'll have to use a browser, or
>> they'll still be able to post-commit review.
>>
>
> It's not just a matter of hypothetical reviewers, but also of documenting
> the review in the public system of record for participants, lurkers, and
> archaeologists who may need to discover why certain choices were made after
> the fact. This was always the plan for Phab - that email was still the
> system of record.
>
>
> Yes, and we agree llvm-commits *should* be there from the start, however
> in the situation I described above, some round of reviews are not and won't
> be available on the list, even if you start a new revision.
>
>
> I suppose it comes down to this: I think the best thing would be for a new
> thread to be created, and the feedback/context ported over from here. So
> it's got a fresh start and is easy to access for current and future readers
> (rather than having to come in half way, trawl through the thread, find the
> original context provided part-way through, etc). There would be some
> inconvenience for existing reviewers, but much of that could be mitigated
> by the original patch author porting over the context into that new thread.
>
> Not as good (for the review, the community, etc), but probably sufficient,
> would be to add the original context to this thread to get everyone up to
> speed on the mailing list.
>
>
> You'll probably have to expect me ranting about it anytime it happens on a
> review I'm involved with :)
>

Then perhaps you can help to instill diligence in those who send reviews
your way to ensure/by ensuring they have been addressed to *-commits before
beginning the review. My particular mail rule setup makes this mistake very
obvious - but I imagine there are other ways to make it obvious to you
depending on how you choose to have your mail rules configured.

- Dave

>
> --
> Mehdi
>
>
>
>
>
>>
>>
>
>>
>>
>>>
>>>>
>>>> A revision on Phabricator that has no history can be nuked without
>>>> consequence of course...
>>>>
>>>> --
>>>> Mehdi
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> >> That said the patch is always a mere two-clicks away from the
>>>>> email: http://
>>>>> >> reviews.llvm.org/D20260?download=true
>>>>> >> One could even write a plugin for his email client to act on link
>>>>> in the email
>>>>> >> "http://reviews.llvm.org/D20260" and automatically curl the latest
>>>>> patch.
>>>>> >
>>>>> > How do I know if I need or want to look at it? Is "Minimise the diff
>>>>> a
>>>>> > little" an interesting patch?
>>>>>
>>>>> Seriously?? The email title says "IR: Introduce local_unnamed_addr
>>>>> attribute", that pretty clear IMO.
>>>>>
>>>>> >
>>>>> >>    There is still the fact that even the initial phab revision has
>>>>> >>    essentially no context. I've skimmed thin three or four times
>>>>> and I'm not
>>>>> >>    sure yet what the motivation is... I'm sure it has one, I just
>>>>> can't find
>>>>> >>    it.
>>>>> >>
>>>>> >> s/Initial phab revision/current phab revision/
>>>>> >>
>>>>> >> Yes, I'd expect a phabricator revision to have a correct
>>>>> description.
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160518/e7a21ee1/attachment.html>