<div dir="ltr"><div>Hi Levente,<div>IMO the undue sqInt cast indeed qualifies as a BUG, we were just lucky enough to not encounter it.</div><div>(said another way, since we have not much tests, we would have corrected it only if encountered it).</div><div><br></div><div>This happens in #generateShiftRight:on:indent: because the type analysis is insufficient.</div><div>You can see that we care to verify if operand is already 64 bits with</div><div> (self is64BitIntegralVariable: msgNode receiver typeInto: [:t| type := t])</div><div>Unfortunately, as the name tells, this method restrict the analysis to the case where</div><div> node isVariable</div><div><br></div></div><div>Note how the analysis of operand type is more careful in generateShiftLeft:on:indent:<div>We changed it several times in 2016 with Eliot, and I finally brought current complexification circa July 2016</div><div>See for example <a href="http://source.squeak.org/VMMaker/VMMaker.oscog-nice.1904.diff">http://source.squeak.org/VMMaker/VMMaker.oscog-nice.1904.diff</a></div><div>(some details improving handling of literal constants have changed since).</div><div><br></div><div>It uses more general C expression type #typeFor:in: rather than just C variable type.</div><div>Eliot wrote this more generic type analyzer and I completed it in 2016, this is posterior the #generateShiftRight:on:indent: time stamp, hence the limitations of #generateShiftRight:on:indent. As said above, we did not require a better one until now...<br></div></div><div><br></div><div>So it seems like we have enough material to correct the generation and make it right.</div><div>We should really pick the Unit Test initiative from Pharo Team.</div><div>A pity, a totally unfair attitude, and a total waste that it's not retro contributed... <br></div><div><br></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le lun. 9 mars 2020 à 18:27, Levente Uzonyi <<a href="mailto:leves@caesar.elte.hu">leves@caesar.elte.hu</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Hi Eliot,<br>
<br>
On Mon, 9 Mar 2020, Eliot Miranda wrote:<br>
<br>
> <br>
> Hi Levente,<br>
><br>
><br>
>> On Mar 9, 2020, at 8:54 AM, Levente Uzonyi <<a href="mailto:leves@caesar.elte.hu" target="_blank">leves@caesar.elte.hu</a>> wrote:<br>
>> <br>
>> Hi All,<br>
>> <br>
>> I wrote a new plugin the other day and found two issues with code generation.<br>
>> The first one, which is less important is related to the inline pragma.<br>
>> I expected that methods marked with <inline: true> will either not be generated at all when the code generator inlines all occurrences of them or they'll be generated as inline C functions. But they are generated as regular static functions even though they are never used.<br>
><br>
> Slang is not always able to inline. For a example, inlining into conditionals is problematic. For this reason Slang treats <inline: true> as a recommendation. If you want to insist, eliminate the static functions, and deal with any failures to inline by suitable rewrites then use <inline: #always>.<br>
><br>
> Slang also obeys <inline: #never> which is useful in other ways (for example reducing code duplication).<br>
<br>
Thanks. <inline: #always> does just what I need.<br>
<br>
><br>
>> The other one, which is more significant is related to type casts.<br>
>> I've got the following method:<br>
>> <br>
>> rotateRight64: value by: amount<br>
>><br>
>> <inline: true><br>
>> <var: #value type: #'unsigned long long'><br>
>> <returnTypeC: #'unsigned long long'><br>
>><br>
>> ^(value >> amount) bitOr: (value << (64 - amount))<br>
>> <br>
>> The code using the above method has the variable declaration<br>
>><br>
>> <var: #w declareC: 'unsigned long long w[80]'><br>
>> <br>
>> The method is used with individual elements of w:<br>
>><br>
>> self rotateRight64: (w at: i - 2) by: 61<br>
>> <br>
>> The generated function gets inlined as expected, but the elements of the array are casted to usqInt before right shifts:<br>
>><br>
>> ((((usqInt) (w[i2 - 2])) >> 61) | ((w[i2 - 2]) << (3)))<br>
>> <br>
>> This is a problem on 32-bit platforms, because usqInt is unsigned int there, so the upper 32-bits will be lost by the cast.<br>
>> <br>
>> When the method is used with non-array variables, then the generator works as expected:<br>
>><br>
>> self rotateRight64: a by: 39<br>
>> <br>
>> is generated as:<br>
>><br>
>> ((a >> 39) | (a << (25)))<br>
>> <br>
>> where a is declared as<br>
>><br>
>> <var: #a type: #'unsigned long long'><br>
>> <br>
>> I tried to fix the issue by declaring w as<br>
>><br>
>> <var: #w type: #'unsigned long long*'><br>
>> <br>
>> But is didn't make the usqInt casts go away.<br>
>> Is it possible that the code generator doesn't know that the type of w[i] is unsigned long long, when w is unsigned long long*?<br>
><br>
> I’ll need to take a look at this in detail. For now, try using the type #usqLong and see if it makes a difference.<br>
<br>
Unfortunately, the code behaves the same way with #usqLong.<br>
<br>
I forgot to mention a third issue. There's no easy way to check the type <br>
of 64-bit and 16-bit arrays. There's #isWords: for 32-bit arrays, and <br>
#isBytes: for 8-bit arrays.<br>
#isWordsOrBytes: is a misnomer now, because it returns true for 8, 16, 32 <br>
and 64-bit arrays.<br>
<br>
I used the following workaround:<br>
<br>
((interpreterProxy isIndexable: hashOop)<br>
and: [ (interpreterProxy isWordsOrBytes: hashOop)<br>
and: [ (interpreterProxy slotSizeOf: hashOop) = 8 ] ])<br>
ifFalse: [ ^interpreterProxy primitiveFailFor: PrimErrBadArgument ].<br>
<br>
The first check is probably unnecessary.<br>
<br>
SpurMemoryManager understands #isLong64s:, but InterpreterProxy doesn't <br>
have that method.<br>
Also, some implementations in InterpreterProxy may be incorrect. e.g. <br>
#isWordsOrBytes:.<br>
<br>
<br>
Levenete<br>
<br>
><br>
>> Levente<br>
>> <br>
>> P.S.: The code is available here <a href="http://squeaksource.com/Cryptography/CryptographyPlugins-ul.18.mcz" rel="noreferrer" target="_blank">http://squeaksource.com/Cryptography/CryptographyPlugins-ul.18.mcz</a> .<br>
>> Generate SHA2Plugin to see the problem.</blockquote></div>