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


_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Reply via email to