[cfe-dev] [LLVMdev] RFC: TileGX, a new backend for Tilera's many core processor

[Dmitri Gribenko]

On Fri, Mar 1, 2013 at 4:52 PM, Jiong Wang <jiwang at tilera.com> wrote:
> On 03/01/2013 10:42 PM, Hal Finkel wrote:
>>
>>
>> As some of the llvm modules are in active development, for example MC
>> Layer, we want to return code to community repository first, so that
>> it will be easy to keep pace with llvm main tree.
>> I think this makes sense; but my impression is that the community will
>> want a clear idea that this will be maintained and improved for the
>> foreseeable future.
>
>      Hi Hal,
>
>        sure, tilegx will be actively maintained & improved.
>
>>>
>>>
>>> 2. There are no regression tests -- using the test-suite is obviously
>>> useful, but targeted regression tests are essential. yes, regression
>>> tests will be added later.
>>
>> I don't speak for everybody, but I'm pretty sure that you won't get an
>> okay to commit this upstream without regression tests. We need a good degree
>> of coverage in test/CodeGen/Tile (you can probably look at one of the other
>> more-recent targets, like AArch64, to get an idea of what is required).
>
>        thanks for pointting this out, I will add regression tests, then
> re-send the patch.
>        before this, please feel free to give comments and suggestions on the
> existed patches.

You also need tests for Clang bits, too.

Mechanical issues:

+/// getTileRegisterNumbering - Given the enum value for some register,
+/// return the number that it corresponds to.

Please don't duplicate function and class name in comments.  Existing
code does this, but current style guidelines advise not to.

http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

+      BuildMI(MBB, I, I->getDebugLoc(), TII->get(Tile::ADD)
+              ,I->getOperand(0).getReg())
+                .addReg(Tile::ZERO)
+                 .addReg(src_sp);

+unsigned TileInstrInfo::
+isLoadFromStackSlot(const MachineInstr *MI, int &FrameIndex) const

You have some weird formatting here and there.  You could try
clang-format to pretty-print your code automatically.

The patch is also missing documentation bits.  At the very least, a
paragraph for ReleaseNotes.

+    // save lr to caller's stack reserve slot 0

Comments should start with a capital letter and end with a full stop.

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>*/