On Fri, 2009-01-16 at 10:03 +0100, Duncan Sands wrote:> Hi Sanjiv, > > > Well the magnitude of the task is not small. > > ExpandIntegerOperand() calls LowerOperation() to allow targets to handle > > illegal operands. So we will need to change the interface of > > LowerOperation() to pass an extra argument called Results, which is an > > array of SDValue. Targets will push the result values in this array and > > then we can replace values in ExpandIntegerOperand(). Very much like > > what CustomLowerResults() and ReplaceNodeResults() are doing currently. > > > > The problem is that do we want to change calls to LowerOperation() in > > LegalizeDAG as well? I think probably that is the right approach to go > > in the longer term. But currently I suggest that "Results" be the last > > argument to LowerOperation() which is defaulted to NULL. That way > > LegalizeDAG and all targets will continue to work the current way, plus > > targets like ours that want to use the last argument (i.e. "Results") > > can use them in ExapndIntegerOperand(). > > do you need this for operation legalization (LegalizeDAG) as well as > type legalization? If not, then you can introduce a new method like > ReplaceNodeResults for custom type legalization of operands (or just > use ReplaceNodeResults for this too - I don't immediately see any > reason why not), and have it call LowerOperation by default. Actually, > if you also want this for LegalizeDAG too, you can introduce a new method > which is only called in places that need it; everywhere else can still > use LowerOperation (of course in the long term there should be just one > method, but this way you can do things incrementally). I don't much > like the idea of having Results be NULL. I'd rather the interface was > uniform, and have any tricks be in the body of the method. > > Ciao, > > Duncan.Thanks Duncan for your suggestions. We have worked out a patch accordingly. The patch is attached, let me know if it looks okay to commit. Thanks, Sanjiv -------------- next part -------------- A non-text attachment was scrubbed... Name: patch.txt Type: text/x-patch Size: 10530 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090119/ecff7e12/attachment.bin>
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> + /// 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. Also, is there any reason not to use ReplaceNodeResults rather than introducing a new method for type legalization?> + 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. Also, you could just make DAGTypeLegalizer::CustomLowerResults more general, and use that. Ciao, Duncan.
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 -------------- A non-text attachment was scrubbed... Name: patch.txt Type: text/x-patch Size: 12606 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090119/a2cc7156/attachment.bin>