llvm dev - Feb 2008 - [LLVMdev] tblgen and sign-extended constants too large for type

Question: How hard should tblgen try to fit constants into a particular
type?

My case is an xor with i8 immediate where I'm using 0xff in as the
immediate value. 0xff should fit into i8 if treated as unsigned, but
CodeGenDAGPatterns.cpp assumes that any and all integers less than
32-bits are signed.

Should tblgen try to see if the sign-extended version of the constant
could fit into the type, and failing that, try the unsigned version:

Index: utils/TableGen/CodeGenDAGPatterns.cpp
==================================================================---
utils/TableGen/CodeGenDAGPatterns.cpp	(revision 8028)
+++ utils/TableGen/CodeGenDAGPatterns.cpp	(working copy)
@@ -685,10 +685,17 @@
           // Make sure that the value is representable for this type.
           if (Size < 32) {
             int Val = (II->getValue() << (32-Size)) >>
(32-Size);
-            if (Val != II->getValue())
-              TP.error("Sign-extended integer value '" +
itostr(II->getValue())+
-                       "' is out of range for type '" +
-                       getEnumName(getTypeNum(0)) + "'!");
+            if (Val != II->getValue()) {
+	      // If sign-extended doesn't fit, does it fit as unsigned?
+	      unsigned ValueMask = unsigned(MVT::getIntVTBitMask(VT));
+	      unsigned UnsignedVal = unsigned(II->getValue());
+
+	      if ((ValueMask & UnsignedVal) != UnsignedVal) {
+		TP.error("Integer value '" + itostr(II->getValue())+
+			 "' is out of range for type '" +
+			 getEnumName(getTypeNum(0)) + "'!");
+	      }
+	    }

Allow me to rephrase my question: How much agony and gnashing of  
teeth will I cause if I commit this patch to tblgen (fully tested and  
changes to DAGISelEmitter.cpp also committed)?


-scooter

On Feb 8, 2008, at 6:43 PM, Scott Michel wrote:
> Question: How hard should tblgen try to fit constants into a  
> particular
> type?
>
> My case is an xor with i8 immediate where I'm using 0xff in as the
> immediate value. 0xff should fit into i8 if treated as unsigned, but
> CodeGenDAGPatterns.cpp assumes that any and all integers less than
> 32-bits are signed.
>
> Should tblgen try to see if the sign-extended version of the constant
> could fit into the type, and failing that, try the unsigned version:
>
> Index: utils/TableGen/CodeGenDAGPatterns.cpp
> ==================================================================> ---
utils/TableGen/CodeGenDAGPatterns.cpp	(revision 8028)
> +++ utils/TableGen/CodeGenDAGPatterns.cpp	(working copy)
> @@ -685,10 +685,17 @@
>            // Make sure that the value is representable for this type.
>            if (Size < 32) {
>              int Val = (II->getValue() << (32-Size)) >>
(32-Size);
> -            if (Val != II->getValue())
> -              TP.error("Sign-extended integer value '" +
> itostr(II->getValue())+
> -                       "' is out of range for type '" +
> -                       getEnumName(getTypeNum(0)) + "'!");
> +            if (Val != II->getValue()) {
> +	      // If sign-extended doesn't fit, does it fit as unsigned?
> +	      unsigned ValueMask = unsigned(MVT::getIntVTBitMask(VT));
> +	      unsigned UnsignedVal = unsigned(II->getValue());
> +
> +	      if ((ValueMask & UnsignedVal) != UnsignedVal) {
> +		TP.error("Integer value '" + itostr(II->getValue())+
> +			 "' is out of range for type '" +
> +			 getEnumName(getTypeNum(0)) + "'!");
> +	      }
> +	    }
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Scott Michel wrote:
> Allow me to rephrase my question: How much agony and gnashing of  
> teeth will I cause if I commit this patch to tblgen (fully tested and  
> changes to DAGISelEmitter.cpp also committed)?
I tested the aforementioned patch in the test subdirectory and the tests
all passed.

The basic implication from this patch is that all constants emitted by
tblgen will be uint64_t hex constants and tblgen won't complain when a
sign-extended value won't fit i16 or i8 or i(i<32-bit) type even though
it would if it were treated as unsigned.


-scooter
 (who's not going to beg forgiveness if the patch isn't Lattnerized(tm))

My feeling is tblgen shouldn't make assumptions about whether the  
backend will treat the value as signed or unsigned (after all  
MVT::ValueType does not convey sign information). Perhaps it's cleaner  
to just check if it fits as unsigned? Chris?

Evan

On Feb 8, 2008, at 6:43 PM, Scott Michel wrote:
> Question: How hard should tblgen try to fit constants into a  
> particular
> type?
>
> My case is an xor with i8 immediate where I'm using 0xff in as the
> immediate value. 0xff should fit into i8 if treated as unsigned, but
> CodeGenDAGPatterns.cpp assumes that any and all integers less than
> 32-bits are signed.
>
> Should tblgen try to see if the sign-extended version of the constant
> could fit into the type, and failing that, try the unsigned version:
>
> Index: utils/TableGen/CodeGenDAGPatterns.cpp
> ==================================================================> ---
utils/TableGen/CodeGenDAGPatterns.cpp	(revision 8028)
> +++ utils/TableGen/CodeGenDAGPatterns.cpp	(working copy)
> @@ -685,10 +685,17 @@
>           // Make sure that the value is representable for this type.
>           if (Size < 32) {
>             int Val = (II->getValue() << (32-Size)) >>
(32-Size);
> -            if (Val != II->getValue())
> -              TP.error("Sign-extended integer value '" +
> itostr(II->getValue())+
> -                       "' is out of range for type '" +
> -                       getEnumName(getTypeNum(0)) + "'!");
> +            if (Val != II->getValue()) {
> +	      // If sign-extended doesn't fit, does it fit as unsigned?
> +	      unsigned ValueMask = unsigned(MVT::getIntVTBitMask(VT));
> +	      unsigned UnsignedVal = unsigned(II->getValue());
> +
> +	      if ((ValueMask & UnsignedVal) != UnsignedVal) {
> +		TP.error("Integer value '" + itostr(II->getValue())+
> +			 "' is out of range for type '" +
> +			 getEnumName(getTypeNum(0)) + "'!");
> +	      }
> +	    }
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Feb 11, 2008, at 11:08 PM, Evan Cheng wrote:
> My feeling is tblgen shouldn't make assumptions about whether the  
> backend will treat the value as signed or unsigned (after all  
> MVT::ValueType does not convey sign information). Perhaps it's  
> cleaner to just check if it fits as unsigned? Chris?
I don't see why there is an issue.  Can't you just write i8 -1  ?

-Chris