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

[Tom Stellard]

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