[LLVMdev] PIC16 backend for llvm 2.5

[Sanjiv.Gupta at microchip.com]

Duncan,
We have code freeze approaching. Should I check-in this revised patch?
I ran make check and it passes.

- Sanjiv


-----Original Message-----
From: llvmdev-bounces at cs.uiuc.edu on behalf of sanjiv gupta
Sent: Mon 1/19/2009 11:51 PM
To: Duncan Sands
Cc: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] PIC16 backend for llvm 2.5
 
On Sun, 2009-01-18 at 20:28 +0100, Duncan Sands wrote:
> Hi Sanjiv,
> 
> > +  /// CustomLowerOperation - This callback is invoked for operations that are
> > +  /// unsupported by the target, which are registered to use 'custom' lowering,
> > +  /// and whose defined values are all legal.
> 
> and whose defined values are all legal -> and whose return values all have legal types
> 
Taken care of.

> > +  /// If the target has no operations that require custom lowering, it need not
> > +  /// implement this.
> 
> This comment and the name seem too generic for me.  This method is for use when
> an operand has an illegal type, right?  Yet the comment makes no mention of this.
> There's also of no mention of the fact that you are allowed to not place anything
> in Results, and what that means.
> 
Taken care of.

> Also, is there any reason not to use ReplaceNodeResults rather than introducing
> a new method for type legalization?
> 
ReplaceNodeResults and LowerOperation have been used for different
purposes by different targets, and have been implemented differently.
The former is meant to legalize illegal value types and the latter is
meant to legalize operations with illegal operands. So they might need
different treatments.

> > +    case ISD::ANY_EXTEND:   
> > +      Results.push_back(PromoteIntOp_ANY_EXTEND(N)); break;
> 
> This is wrong if PromoteIntOp_ANY_EXTEND returned a value with
> no node.  Likewise for all the others.  Better I think to simply
> handle the custom case immediately and return rather than trying
> to share code with these other cases.
> 
Taken care of.

> Also, you could just make DAGTypeLegalizer::CustomLowerResults
> more general, and use that.
> 
Taken care of.

The revised patch is attached.

> Ciao,
> 
> Duncan.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090120/26dd8d91/attachment.html>