don't store UntypedInt with 4-bit APInt values #125
Conversation
|
Google » souper #338 SUCCESS |
|
Yang, thanks! I don't see a problem with 8 bits but if I understand correctly, 5 would be enough? |
|
Yang can you redo for 5 bits and squash? Thanks! |
|
5 bits should be enough. Updated. Thanks. |
|
Wait a second. I will need to update the commit message. Sorry about that! |
|
Google » souper #341 SUCCESS |
Integer 8 is represent as b1000 in a 4-bit APInt manner. Later, when we translate this APInt value into a const using APInt.sextOrTrunc, we get b11111000 back, which is not what we want. The bad intepretation only occurs when we infer the type of an integer, i.e., an integer without :iN. Note that b11111000 is unsigned char 248, which is why we see instruction "%8:i1 = eq 248:i8, %0" in John's example. The fix is to use multiple of 5. It should always be safe. Also, we won't increase too much memory usage because it's only for parsing replacements, which are small.
|
John, pull request has been updated. Thanks! |
|
Google » souper #342 SUCCESS |
don't store UntypedInt with 4-bit APInt values
|
Thanks Yang! |
|
Google » souper #343 SUCCESS |
| std::string Test; | ||
| std::string ExpectedReplacement; | ||
| } Tests[] = { | ||
| { R"i(%0:i8 = var ; 0 |
There was a problem hiding this comment.
Could this be a RoundTrip test?
There was a problem hiding this comment.
Unless I missed something, we can't simply include the test into RoundTrip category. The input is "%4:i1= eq 8, %0 ", and the output is "%4 = eq 8:i8, %0". The bug occured when we inferred the width of "8", so we can't add :i8 to it. On the other hand, i8 was printed out by our Inst printer. This is way I explicitly added one more test here.
There was a problem hiding this comment.
Never mind. I see that RoundTrip test has similar functionality for checking equivalence. I overlooked it.
There was a problem hiding this comment.
We could add it to NonEqualTests, no?
There was a problem hiding this comment.
Yes, I saw that. Thanks!
There was a problem hiding this comment.
Go ahead if you want to, Yang, I was going to do this earlier but something came up.
|
OK, I am on it . Thanks. |
Integer 8 is represent as b1000 in a 4-bit APInt manner.
Later, when we translate this APInt value into a const using
APInt.sextOrTrunc, we get b11111000 back, which is not what
we want. The bad intepretation only occurs when
we infer the type of an integer, i.e., an integer without :iN.
Note that b11111000 is unsigned char 248, which is why we see
instruction "%8:i1 = eq 248:i8, %0" in John's example.
The fix is to use multiple of 5. It should always be safe.
Also, we won't increase too much memory usage because
it's only for parsing replacements, which are small.