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

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.
---
Regards,
Jiong
>
>   -Hal
>
>>
>> You say that there are unexpected failures in the regression tests
>> and in the test suite. Do you understand these? yes, I have done
>> investigation on these failures.
>>
>> regression (44 failures)
>> ==>> 20: caused JITTests failures, we lack old jit support
>> 2:
>> MCJIT/test-common-symbols-remote.ll
>> MCJIT/test-fp-no-external-funcs-remote.ll
>>
>> the other 20 are mostly under Clang:: Tooling (these testcases seems
>> to be added in this week) & some debuginfo testcases, I will clean
>> up them later.
>>
>>
>> test-suite (17 failures)
>> =====>> 1 of them are compile stage failures
>> ==>> MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4
>> the reason is tilegx do not support some c99 float rounding &
>> exceptions
>> we lack the support of some flag like FE_DIVBYZERO etc, for
>> fegetexceptflag.
>>
>> 16 of them are runtime failures.
>> ==>> actually, I compare all these failures with our gcc output,
it's the
>> same.
>> nearly all of them are caused by float point precision issue, for
>> example,
>>
>> MultiSource/Applications/sqlite3/
>> ...
>>   6|496148.333333333
>> -6|496234.333333333  <=== expected output & x86_64 result
>> +6|496234.333333334  <=== tilegx gcc/llvm result
>>   8|496373.0
>>   8|496448.375
>>   1|496587.0
>> -6|496647.333333333
>> +6|496647.333333334
>>   ...
>>
>> ---
>> Regards,
>> Jiong
>>
>> -Hal
>>
>> TILE-Gx is a VLIW architecture with 64-bit registers, 64-bit address
>> space,
>> and 64-bit instructions. TILE-Gx has load-store architecture ISAs.
>>
>> More information on the architectures is available at
>> http://www.tilera.com/scm/docs/index.html .
>>
>> the attached patches contains the following main features for tilegx
>> backend:
>>
>> 1. general function.
>> 2. PIC/TLS/JumpTable.
>> 3. Instructoin Bundling for VLIW.
>> 4. Basic support for Asm Parser.
>> 5. MC Layer (aware of VLIW), MCJIT support.
>>
>> I've run the regression test and standalone test-suite natively on
>> TILE-Gx silicon. The
>> test results are:
>>
>> regression
>> ---
>> Expected Passes : 13218
>> Expected Failures : 78
>> Unsupported Tests : 68
>> Unexpected Failures: 44
>>
>> test-suite
>> ---
>> Expected Passes : 949
>> Unexpected Failures: 17
>>
>> the llvm patch is against:
>>
>> commit 5e812139690ce077d568ef6559992b2cf74eb536
>> Author: Evgeniy Stepanov <eugeni.stepanov at gmail.com> Date: Thu
Feb 28
>> 11:25:14 2013 +0000
>>
>> [msan] Implement sanitize_memory attribute.
>>
>> the clang patch is against:
>>
>> commit a4d4621b206f941cc58d9d0bc7c67a8e705c9d49
>> Author: Daniel Jasper <djasper at google.com> Date: Thu Feb 28
11:05:57
>> 2013 +0000
>>
>> Improve formatting of #defines.
>>
>> the test-suite patch is against:
>>
>> commit 8c1ab3b660e67b74421d657408167b1345188f8d
>> Author: Duncan Sands <baldrick at free.fr> Date: Sun Feb 17
15:21:11
>> 2013 +0000
>>
>> Use LLVMCC_EMITIR_FLAG rather than -emit-llvm
>>
>>
>> please review, looking forward to your feedback.
>>
>> thanks.
>>
>> ---
>> Regards,
>> Jiong
>> Tilera Corporation.
>>
>> _______________________________________________
>> LLVM Developers mailing list LLVMdev at cs.uiuc.edu
>> http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>> --
>> Regards,
>> Jiong. Wang
>> Tilera Corporation.

-- 
Regards,
Jiong. Wang
Tilera Corporation.

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

On 03/02/2013 04:50 AM, Dmitri Gribenko wrote:
> 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.
    Hi Damitri,

        thanks for your time to review, I will fix these things according
to llvm coding style doc.

---
Regards,
Jiong
>
> Dmitri
>

-- 
Regards,
Jiong. Wang
Tilera Corporation.