<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><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">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>