Usability of phabricator review threads for non-phab-users

[Tobias Grosser]

On 01/07/2014 13:33, Tobias Grosser wrote:
> On 01/07/2014 13:11, Manuel Klimek wrote:
>> Alp noted that the current setup on how phab reviews land on the list are
>> not working for him. I'd be curious whether his setup is special, or
>> whether there are more widespread problems. If this is more widely
>> perceived as a problem, please speak up, and I'll make sure to prioritize
>> the fixes (note that this is unrelated to the "lost email" problem -
>> those
>> are always highest priority and as far as I'm aware we diagnosed and
>> fixed
>> all of them within 1-2 business days).
>>
>> If you have the feeling that the phab email workflow makes it hard for
>> you
>> to jump into reviews, keep track of reviews, or understand reviews if
>> you're not a phab user, please reply to this thread. You don't need to
>> provide details, "+1", "please fix", or "doesn't work well for me" are
>> all
>> acceptable replies here - I want to get a feeling for the magnitude of
>> the
>> problem.
>
> Hi Manuel,
>
> thanks for looking into this.
>
> I remember me having issues due to patch comments missing relevant
> context when looking at them on the mailing list. I think this has been
> both too little code lines, but possibly also previous comments which
> have not been cited. When people use email, they normally ensure that
> the relevant code/comments are properly cited. Phabricator does not need
> this in the web interface, which makes the emails sometimes less useful.

Below an example of such a "less useful reply".

> -------- Original Message --------
> Subject: Re: [PATCH] [Peephole] Advanced rewriting of copies to avoid cross register banks copies.
> Date: Tue, 1 Jul 2014 13:04:01 +0000
> From: Quentin Colombet <qcolombet at apple.com>
> Reply-To: reviews+D4086+public+3593472fe90901bd at reviews.llvm.org
> To: qcolombet at apple.com
> CC: llvm-commits at cs.uiuc.edu
>
> ================
> Comment at: lib/CodeGen/PeepholeOptimizer.cpp:981
> @@ +980,3 @@
> +    return getNextSourceFromInsertSubreg(SrcIdx, SrcSubReg);
> +  if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)
> +    return getNextSourceFromExtractSubreg(SrcIdx, SrcSubReg);
> ----------------
> hfinkel at anl.gov wrote:
> > Quentin Colombet wrote:
> > > hfinkel at anl.gov wrote:
> > > > I think you might as well add isExtractSubreg().
> > > Agree.
> > > Though, I did not want to do that in that patch.
> > > I can either do it before or after this patch lands.
> > > Any preference?
> > I don't have a strong preference. Personally, I'd do it first.
> I thought there were plenty of users for this accessor but turns out all the users check the opcode of a SDNode whereas I am looking at MachineInstr. Thus, this patch is the first user of isExtractSubreg.
> So I have updated the patch to include that change.
>
> http://reviews.llvm.org/D4086

Here some more context:

   if (Def->isCopy())
     return getNextSourceFromCopy(SrcIdx, SrcSubReg);
   if (Def->isBitcast())
     return getNextSourceFromBitcast(SrcIdx, SrcSubReg);
   // All the remaining cases involve "complex" instructions.
   // Bails if we did not ask for the advanced tracking.
   if (!UseAdvancedTracking)
     return false;
   if (Def->isRegSequence())
     return getNextSourceFromRegSequence(SrcIdx, SrcSubReg);
   if (Def->isInsertSubreg())
     return getNextSourceFromInsertSubreg(SrcIdx, SrcSubReg);
   if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)

Only with the additional context it is visible, that all ifs are of the 
form:

   if (Def->isSomething)

but only the last is of the form:

   if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)

This is what Hal is referring to, but the email message lacks the
context and is consequently not understandable. I understand that you
try to keep the traffic low, but I believe adding context does not cost
a lot, as we can easily skim over it. So more context (+10 lines?)
before (and possibly even after the comment) would be very helpful.

Also, I am missing the function name this change is applied to. diff
is normally very good in finding the name of the function a change is
applied to. This information is lost in the phabricator emails.

And while at it. I think it is unfortunate that the 'author wrote' lines
are between the change and the comment as this reduces the distance 
between the code and the comments. Finally, by using indentation for the
patch itself, mail clients format the citation properly and the temporal 
order of the patch and the comments becomes clear.

Overall, I formatting this email as follows would change this email from 
not understandable to very useful.

hfinkel at anl.gov wrote:
> Quentin Colombet wrote:
> > hfinkel at anl.gov wrote:
> > > Comment at: lib/CodeGen/PeepholeOptimizer.cpp:981
> > > > @@ +980,3 @@ bool boolValueTracker::getNextSourceImpl()
> > > > + if (Def->isBitcast())
> > > > +   return getNextSourceFromBitcast(SrcIdx, SrcSubReg);
> > > > + // All the remaining cases involve "complex" instructions.
> > > > + // Bails if we did not ask for the advanced tracking.
> > > > + if (!UseAdvancedTracking)
> > > > +   return false;
> > > > + if (Def->isRegSequence())
> > > > +   return getNextSourceFromRegSequence(SrcIdx, SrcSubReg);
> > > > + if (Def->isInsertSubreg())
> > > > +    return getNextSourceFromInsertSubreg(SrcIdx, SrcSubReg);
> > > > +  if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)
> > > > +    return getNextSourceFromExtractSubreg(SrcIdx, SrcSubReg);

> > > I think you might as well add isExtractSubreg().
> > Agree.
> > Though, I did not want to do that in that patch.
> > I can either do it before or after this patch lands.
> > Any preference?
> I don't have a strong preference. Personally, I'd do it first.
I thought there were plenty of users for this accessor but turns out all 
the users check the opcode of a SDNode whereas I am looking at 
MachineInstr. Thus, this patch is the first user of isExtractSubreg.
So I have updated the patch to include that change.

> > > > +   if (Def->isSubregToReg())
> > > > +     return getNextSourceFromSubregToReg(SrcIdx, SrcSubReg);
> > > > +   return false;
> > > > + }
> > > > +

Cheers,
Tobias