PIC16 backend for llvm 2.5

Hi Duncan,

We are targetting a reasonably functional PIC16 backend for llvm 2.5.
The only problem in our way is a local patch in ExpandIntegerOperand, which couldn’t make its way to trunk so far. The discussion is contained in the following link:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20081103/069593.html

I now have time to take it up again and do whatever rework is required. I saw that you have made some changes in the legalizer recently; let me know if the same discussion in the above thread still holds good and whether we can start working on it as described therein.

Thanks

  • Sanjiv

Hi Sanjiv,

We are targetting a reasonably functional PIC16 backend for llvm 2.5.
The only problem in our way is a local patch in ExpandIntegerOperand, which couldn't make its way to trunk so far. The discussion is contained in the following link:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20081103/069593.html

I now have time to take it up again and do whatever rework is required. I saw that you have made some changes in the legalizer recently; let me know if the same discussion in the above thread still holds good and whether we can start working on it as described therein.

I don't remember what the discussion was about any more. The link doesn't
contain much. Anyway, as you may have noticed I changed the definition of
the target hook "ReplaceNodeResults". Probably the same method, or one
with a similar definition, should be used for custom lowering in the case
of an illegal operand. Would this solve your problem?

Ciao,

Duncan.

Well, the first email is here.

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20081013/068667.html

Hi Sanjiv,

Well, the first email is here.

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20081013/068667.html

thanks, I remember now (more or less). So would something like ReplaceNodeResults
solve the problem?

Ciao,

Duncan.

Well, the first email is here.

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20081013/068667.html

thanks, I remember now (more or less). So would something like ReplaceNodeResults
solve the problem?

I think it will. We will try that out and post changes.
Thanks for all your help.

Regards,
Sanjiv

From: Duncan Sands [mailto:baldrick@free.fr]
Sent: Friday, January 09, 2009 5:23 PM
To: Sanjiv Kumar Gupta - I00171
Cc: llvmdev@cs.uiuc.edu
Subject: Re: PIC16 backend for llvm 2.5

Hi Sanjiv,

> Well, the first email is here.
>
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-
20081013/068667.html

thanks, I remember now (more or less). So would something like
ReplaceNodeResults
solve the problem?

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().

Let me know if that sounds okay.

Regards,
Sanjiv

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

patch.txt (10.3 KB)

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.

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.

patch.txt (12.3 KB)

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

  • Sanjiv