[llvm-commits] [llvm-gcc] Cast Fix

[Reid Spencer]

Chris, 

You asked for a case where I need to (recursively) determine the sign of
a some Value. I have a case in mind:
lib/Transforms/Utils/LowerSwitch.cpp.  When it decides to lower this to
a branch tree it is recursively calling switchConvert and passing the
switch value in. The signedness is needed in order to generate the
ICmpInst with either SLT or ULT for the branch tree. But that Value*
being tested originates from operand 0 of the switch instruction.
There's no way to determine if its signed or unsigned without starting
to look at it. As I noted in my example below, it could take several
steps of tracking through instructions before I come up with a
definitive answer. And there may be no way to determine it statically if
that search ends up at an argument, global variable, or constant.

Can we just make switch staetments always use SLT for the comparison and
have the issue go away? 

Reid.

On Mon, 2006-12-18 at 23:15 -0800, Chris Lattner wrote:
> >> You're basing all of the checks here on whether or not the LLVM types
> >> are signed.  I understand that this is the most expedient way to get
> >> stuff working in the short term, but this isn't going to work long
> >> term, as the ->isSigned() methods are not going to exist any more.
> >> You should eventually migrate these to using TREE_UNSIGNED and
> >> similar macros, checking properties from the GCC trees.
> >
> > Yes, I understand. I was solely working towards a) fixing the breakage
> > and b) retaining current functionality. There were a lot of changes to
> > make in a short time to get llvm-gcc compiling again. Many of these  
> > need
> > some deep analysis and rather than delay the patch, I took this route.
> 
> Yep, makes sense.
> 
> >>
> >> Further, the casts in i386.h that call getCastOpcode, should be
> >> updated to use the appropriate cast.  However, I'm not sure what they
> >> are, you can work with Evan to figure them out.
> >
> > I tried a few of them as specific casts and it broke many things which
> > is just an indication that I didn't fully analyze the set of types  
> > that
> > can be passed in. The places were getCastOpcode will remain in use are
> > those places where the type could be anything or close to anything  
> > (e.g.
> > function arguments and returns).  These also will be reviewed in more
> > detail as I continue to remove signlessness.
> 
> Ok.
> 
> > FYI: my strategy here is to work from the bottom up. With the last few
> > patches VMCore contains only one isSigned() call (in AsmWriter). I'm
> > currently working on a fix for that. Once VMCore is clean, I'll  
> > move on
> > to other libraries and on up the chain, removing getCastOpcode,
> > isSigned, isUnsigned, getSignedVersion, getUnsignedVersion, etc.
> 
> Nice.
> 
> > This will take a while, many of the remaining cases require some
> > significant modifications in order to figure out if the value is  
> > signed
> > or not.  Speaking of which, I was thinking of adding a method to
> > Instruction, isSignedOperand(unsigned) which recursively figures  
> > out if
> > the operand can be signed. Consider a bunch of arithmetic based on a
> > ZExt or SExt, something like:
> >
> > bool %func(sbyte %a) {
> >    %b = zext sbyte %a to short
> >    %c = sext sbyte %a to ushort
> >    %d = add short %b, %b
> >    %e = add ushort %c, %c
> >    %f = bitcast short %d to ushort
> >    %ret = call bool %doit(ushort %f, short %d)
> >    ret bool %ret
> > }
> >
> > In such a function the declared types are meaningless. %b is  
> > unsigned. %
> > c is signed. When it comes time to check the parameters to %doit, I  
> > need
> > to know that. About the only way I can think of is to write a function
> > on Instruction that recursively calls itself following the use chain
> > until it finds something that is concretely signed or not. Even this
> > might not help. Suppose the zext and sext weren't in this function. I
> > can't statically determine the signedness of sbyte %a which might be
> > signed or not.
> 
> I don't think this is a good idea.  What specific clients would use  
> this?
> 
> -Chris