<div dir="ltr">I don't think #2 is a correct change.  Just because the sign bit is set doesn't mean it's signed.  Is the 4-byte value 0x10000000 signed or unsigned?  It's a trick question, because there's not enough information!  If it was written "int x = 0x10000000" then it's signed (and negative).  If it was written "unsigned x = 0x10000000" then it's unsigned (and positive).  What about the 4-byte value 0x1?  Still a trick!  If it was written "int x = 1" then it's signed (and positive), and if it was written "unsigned x = 1" then it's unsigned (and positive).<div><br></div><div>My point is that signedness of the *type* does not necessarly imply signedness of the value, and vice versa.</div><div><br></div><div>APInt is purely a bit-representation and a size, there is no information whatsoever about whether type *type* is signed.  It doesn't make sense to say "is this APInt negative?" without additional information.</div><div><br></div><div>With APSInt, on the other hand, it does make sense to ask that question.  If you have an APSInt where isSigned() is true, *then* you can use the sign bit to determine whether it's negative.  And if you have an APSInt where isSigned() is false, then the "sign bit" is not actually a sign bit at all, it is just an extra power of 2 for the unsigned value.</div><div><br></div><div>This is my understanding of the classes, someone correct me if I'm wrong.</div><div><br></div><div>IIUC though, the way to fix this is by using APSInt throughout the class, and delete all references to APInt.</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jan 4, 2019 at 2:58 PM Jonas Devlieghere <<a href="mailto:jonas@devlieghere.com">jonas@devlieghere.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>If I understand the situation correctly I think we should do both. I'd start by doing (2) to improve the current behavior and add a constructor for APSInt. We can then audit the call sites and migrate to APSInt where it's obvious that the type is signed. That should match the semantics of both classes?</div></div><div dir="ltr"><div><br></div><div class="gmail_quote"><div dir="ltr">On Fri, Jan 4, 2019 at 2:00 PM Davide Italiano <<a href="mailto:dccitaliano@gmail.com" target="_blank">dccitaliano@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On Fri, Jan 4, 2019 at 1:57 PM Davide Italiano <<a href="mailto:dccitaliano@gmail.com" target="_blank">dccitaliano@gmail.com</a>> wrote:<br>
><br>
> While adding support for 512-bit integers in `Scalar`, I figured I<br>
> could add some coverage.<br>
><br>
> TEST(ScalarTest, Signedness) {<br>
>  auto s1 = Scalar(APInt(32, 12, false /* isSigned */));<br>
>  auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));<br>
>  ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails<br>
>  ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass<br>
> }<br>
><br>
> The result of `s1.GetType()` is Scalar::e_sint.<br>
> This is because an APInt can't distinguish between "int patatino = 12"<br>
> and "uint patatino = 12".<br>
> The correct class in `llvm` to do that is `APSInt`.<br>
><br>
<br>
Please note that this is also broken in the case where you have<br>
APInt(32 /* bitWidth */, -323);<br>
because of the way the constructor is implemented.<br>
<br>
--<br>
Davide<br>
</blockquote></div></div></blockquote></div>