[LLVMdev] [patch] MicroBlaze Backend

[Wesley Peck]

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