On Jan 30, 2010, at 6:49 AM, Anton Korobeynikov wrote:
>> Your patch looks very clean. Some comments:
> Heh, Jakob was faster :)
I have taken care of everything Jakob mentioned except the extra test cases. I
will get to these as soon as I can.
>
>> - I think you have some literal tabs in your instruction descriptions.
> The tabs can be seen in some other places as well. Also, there is a
> "mix" of coding conventions in the files. It will be really nice
to
> use only one :)
I cherry-picked a lot of code from the Mips, Sparc, and PowerPC backends.
That's probably why there are multiple conventions in use. I will try to
clean this up as I continue to make changes.
>
>> - Your tests are nice, but you could use some more of them. I would
recommend tests for tricky calling convention stuff, some loops, switches and
branches. Also indirect calls and indirect branches. Have you tries running
Codegen/generic through your back end? That should give you some more test
cases.
> Some "feature" tests can be found inside other directories as
well,
> e.g. msp430 / systemz.
>
> Are you planning to add microblaze description to clang? C compiler
> can speed up testing alot :)
I already have a microblaze description in clang but I have not tested it as
much so I have not submitted a patch for it yet.
>
> More comments:
>> +SDValue MBlazeTargetLowering::
>> +LowerDYNAMIC_STACKALLOC(SDValue Op, SelectionDAG &DAG) {
> Do you really need this? Expanding dynamic allocas normally ends with
> stack register adjustment, you don't need anything special here.
I was unsure as to whether I needed this or not. I have been taking the approach
of removing things conservatively as I continue to improve the backend. I will
take a look at removing this for the next patch.
>
>> +bool MBlazeTargetLowering::
>> +SelectAddrRegReg(SDValue N, SDValue &Base, SDValue &Index,
SelectionDAG &DAG) {
> Move this to ISelDAGToDAG file
>
>> +bool MBlazeTargetLowering::
>> +SelectAddrRegImm(SDValue N, SDValue &Disp, SDValue &Base,
SelectionDAG &DAG) {
> Likewise
Done.
>
>> +def : Proc<"v400", []>;
>> +def : Proc<"v500", []>;
>> +def : Proc<"v600", []>;
>> +def : Proc<"v700", []>;
>> +def : Proc<"v710", []>;
> What's the difference between them then? isVN00() predicates are not
used.
These have not been completed yet. Some of the earlier processors don't
support all of the instructions but I have not gotten around to enumerating all
of the features that each processor supports. This will probably be done for the
next patch.
>
>> + DataLayout(std::string("E-p:32:32-i8:8:8-i16:16:16-n32")),
> I'm confused. What are the native types for this arch?
The microblaze has only 32-bit registers. Should this be
"E-p:32:32:32-i8:8:32-i16:16:32-n32" instead?
>
>> +static cl::opt<bool> HasFPUOpt(
>> + "mb-has-fpu",
> As Jakob already said - remove these, you should use subtarget
> features (-mattr=+foo) instead.
Done.
>
> As I understand, some features are not hooked yet, like
>> +def FeaturePipe3 : SubtargetFeature<"pipe3",
"HasPipe3", "true",
>> + "Implements 3-stage
pipeline.">;
The microblaze can be configured for either a 3-stage pipeline or a 5-stage
pipeline at system synthesis time. The three stage pipeline has lower area but
also lower performance. This should eventually have an impact on the microblaze
functional unit scheduling, but I have not gotten there yet.
>
> ?
>
> Otherwise patch looks good. Thanks!
>
> --
> With best regards, Anton Korobeynikov
> Faculty of Mathematics and Mechanics, Saint Petersburg State University