[llvm-commits] SETCC Patches #2

[Chris Lattner]

>> It would be better to split up the "EqualTo" predicate here so that
>> there was one for FP and one for integers.  When the FP part of this
>> lands, you'll want "Unordered" "Equal" and "LessThan" instead of just
>> LessThan + Equal.
>
> Okay, good idea. Could you please review the CF.patch file which is
> attached. I think this is what you had in mind.

It looks great.  However, if possible, it sure would be nice to just  
nuke the templates :).

>> Further, 'lessthan' drops the sign, so the code is apparently wrong.
>
> I'm not following this. I thought you weren't allowed to do relational
> operators on packed type? Are you talking about the ConstantInt case?
> This:
>   static Constant *LessThan(const ConstantInt *V1, const ConstantInt
> *V2) {
>     bool R = (BuiltinType)V1->getZExtValue() <
> (BuiltinType)V2->getZExtValue();
>     return ConstantBool::get(R);
>   }

Right, but that LessThan predicate (which always evaluates the  
predicate as unsigned) is also called for slt.  This is a bug.


>>
>> +Constant *llvm::ConstantFoldCompareInstruction(
>> +    unsigned opcode, Constant *C1, Constant *C2, unsigned short
>> predicate) {
>> ...
>> +  case ICmpInst::ICMP_ULT:
>> +  case ICmpInst::ICMP_SLT:   C = ConstRules::get(C1, C2).lessthan 
>> (C1,
>> C2);break;
>> +  case ICmpInst::ICMP_UGT:
>> +  case ICmpInst::ICMP_SGT:   C = ConstRules::get(C1, C2).lessthan 
>> (C2,
>> C1);break;
>> +  case ICmpInst::ICMP_ULE:
>> +  case ICmpInst::ICMP_SLE:   // C1 <= C2  ===  !(C2 < C1)
>> +    C = ConstRules::get(C1, C2).lessthan(C2, C1);
>> +    if (C) return ConstantExpr::getNot(C);
>> +    break;
>> +  case ICmpInst::ICMP_UGE:
>> +  case ICmpInst::ICMP_SGE:   // C1 >= C2  ===  !(C1 < C2)
>> +    C = ConstRules::get(C1, C2).lessthan(C1, C2);
>> +    if (C) return ConstantExpr::getNot(C);
>> +    break;
>>
>>
>> These drop the sign of the comparison, which is a serious bug.  I'd
>> expect this to miscompile things like:
>>
>>
>>  %X = iset slt ubyte 0, ubyte 128
>>
>>
>> ... or whatever the syntax is.
>
> The LessThan code above for ConstantInt works fine. It extracts the
> canonical ZExt form then casts each operand to the correct type before
> doing the compare:
>    bool R = (BuiltinType)V1->getZExtValue() <
> (BuiltinType)V2->getZExtValue();
>
> I don't see the problem.

The code is trying to do a signed less than comparison, you're always  
doing an unsigned comparison.



I will look into 'SETCC InstructionCombining.cpp' next, but not tonight.

-Chris


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061218/457a4a4a/attachment.html>