llvm dev - Dec 2012 - [LLVMdev] [llvm-commits] RFC: Merge branches/R600 into TOT for 3.2 release

On Thu, Nov 29, 2012 at 06:28:15PM +0200, Dmitri Gribenko
wrote:
> On Tue, Nov 27, 2012 at 12:37 AM, Tom Stellard <tom at stellard.net>
wrote:
> > Assuming I caught all the coding style mistakes you pointed out, what
> > else needs to be done to be able to commit this code?
> 
> Hello Tom,
> 
> There are still a few classes of coding style issues...
> 
> +  /// CreateLiveInRegister - Helper function that adds Reg to the LiveIn
list
> +  /// of the DAG's MachineFunction.  This returns a Register SDNode
> representing
> +  /// Reg.
> +  SDValue CreateLiveInRegister(SelectionDAG &DAG, const
> TargetRegisterClass *RC,
> +                                                  unsigned Reg, EVT VT)
const;
> 
> Please don't duplicate function or class name inside the comment.
> Please also don't duplicate the comment in .cpp file.  Old code is
> written that way, but current coding style advises not to duplicate.
> 
> This link may be helpful:
>
<http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments>.
> 
> +#ifndef _AMDIL7XXDEVICEIMPL_H_
> +#define _AMDIL7XXDEVICEIMPL_H_
> 
> Identifiers starting with underscore and a capital letter are reserved.
> 
> +  CFGTraits::insertCondBranchBefore(branchInstrPos,
> +                                   
CFGTraits::getBranchNzeroOpcode(oldOpcode),
> +                                    passRep,
>
+»······»·······»·······»·······»·······»·······»·······»·······»·······branchDL);
> 
> No tabs, please.
> 
> +AMDGPUCFGStructurizer::AMDGPUCFGStructurizer(char &pid, TargetMachine
&tm
> +                                          )
> +: MachineFunctionPass(pid), TM(tm), TII(tm.getInstrInfo()),
> +  TRI(static_cast<const AMDGPURegisterInfo *>(tm.getRegisterInfo())
> +  ) {
> +}
> 
> Both right parentheses should be on previous lines.
> 
> +};   //end of class AMDGPUCFGPrepare
> 
> In general I don't see "end of class" and "end of
function" comments
> in LLVM, just namespaces.
> 
> +// The AMDGPUCypressDevice is similiar to the AMDGPUEvergreenDevice,
> except it has
> +// support for double precision operations. This device is used to
represent
> +// both the Cypress and Hemlock cards, which are commercially known as
HD58XX
> +// and HD59XX cards.
> +class AMDGPUCypressDevice : public AMDGPUEvergreenDevice {
> 
> Lots of comments should have three slashes (to become documentation
comments).
>
Hi Dimitri,

Thanks for taking a look at the code.

I went ahead and made all of these changes, and I reviewed the doxygen section
of the Coding Standards document and updated the comments as well.
I also fixed the issue mentioned by Ben with braces on their own line. 

I've posted an updated patch here:
http://people.freedesktop.org/~tstellar/llvm/Nov29/

Also in that directory are two patches which show the changes since the
previous version of R600.patch.

Hopefully this patch looks a little better.

Thanks,
Tom

On Fri, Nov 30, 2012 at 12:13 AM, Tom Stellard <tom at stellard.net>
wrote:
> I went ahead and made all of these changes, and I reviewed the doxygen
section
> of the Coding Standards document and updated the comments as well.
> I also fixed the issue mentioned by Ben with braces on their own line.
>
> I've posted an updated patch here:
> http://people.freedesktop.org/~tstellar/llvm/Nov29/
>
> Also in that directory are two patches which show the changes since the
> previous version of R600.patch.
>
> Hopefully this patch looks a little better.
Indeed it does!  Thank you!

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at
gmail.com>*/

On Nov 29, 2012, at 2:13 PM, Tom Stellard <tom at stellard.net>
wrote:
> Hi Dimitri,
> 
> Thanks for taking a look at the code.
> 
> I went ahead and made all of these changes, and I reviewed the doxygen
section
> of the Coding Standards document and updated the comments as well.
> I also fixed the issue mentioned by Ben with braces on their own line. 
> 
> I've posted an updated patch here:
> http://people.freedesktop.org/~tstellar/llvm/Nov29/
> 
> Also in that directory are two patches which show the changes since the
> previous version of R600.patch.
> 
> Hopefully this patch looks a little better.
I'm obviously not an expert on this architecture, but the patch looks
basically ok to me.  Please commit!

-Chris

On Mon, Dec 10, 2012 at 11:03:16PM -0800, Chris Lattner
wrote:
> On Nov 29, 2012, at 2:13 PM, Tom Stellard <tom at stellard.net>
wrote:
> > Hi Dimitri,
> > 
> > Thanks for taking a look at the code.
> > 
> > I went ahead and made all of these changes, and I reviewed the doxygen
section
> > of the Coding Standards document and updated the comments as well.
> > I also fixed the issue mentioned by Ben with braces on their own line.
> > 
> > I've posted an updated patch here:
> > http://people.freedesktop.org/~tstellar/llvm/Nov29/
> > 
> > Also in that directory are two patches which show the changes since
the
> > previous version of R600.patch.
> > 
> > Hopefully this patch looks a little better.
> 
> I'm obviously not an expert on this architecture, but the patch looks
basically ok to me.  Please commit!
> 
r169915 \o/