On 28.05.2013 18:18, Richard Henderson wrote:
> On 05/28/2013 08:28 AM, Claudio Fontana wrote:
>> +static inline void tcg_out_movi_aux(TCGContext *s,
>> + TCGReg rd, uint64_t value)
>> +{
>> + uint32_t half, base, movk = 0, shift = 0;
>> +
>> + /* construct halfwords of the immediate with MOVZ/MOVK with LSL */
>> + /* using MOVZ 0x52800000 | extended reg.. */
>> + base = (value > 0xffffffff) ? 0xd2800000 : 0x52800000;
>> +
>> + do {
>> + int skip_zeros = ctz64(value) & (63 & -16);
>> + value >>= skip_zeros;
>> + shift += skip_zeros << 17;
>> + half = value & 0xffff;
>> + tcg_out32(s, base | movk | shift | half << 5 | rd);
>> + movk = 0x20000000; /* morph next MOVZs into MOVKs */
>> + value >>= 16;
>> + shift += 16 << 17;
>
> This is way more confusing than it needs to be. I don't think you
> should modify VALUE by shifting at all. If you don't do that then
> you don't need to make SHIFT loop carried, since we compute its
> exact correct value every time with the ctz.
>
> Was the only bug in the code that I pasted the lack of the shift-by-17
> when encoding SHIFT into the tcg_out32?
yes, you only forgot to encode the shift in the tcg_out32,
the variation above was an attempt to make it easier to understand.
I agree that the approach that avoids changing value in the right shift
is more concise, I'll go back to that, adding a comment about how the
function works.
>> +static inline void tcg_out_movi(TCGContext *s, TCGType type,
>> + TCGReg rd, tcg_target_long value)
>> +{
>> + if (type == TCG_TYPE_I64) {
>> + tcg_out_movi_aux(s, rd, value);
>> + } else {
>> + tcg_out_movi_aux(s, rd, value & 0xffffffff);
>> + }
>> +}
>
> Any reason you're splitting out tcg_out_movi_aux to a separate function?
tcg_out_movi is an interface with tcg, and as such the prototype is fixed.
I'd rather work with a value that is unsigned, because of the right shift.
Having a separate _aux function does that without the need for adding
another local variable and another operation to understand in the function
with the actual algorithm.
>
>> + tcg_regset_clear(s->reserved_regs);
>> + tcg_regset_set_reg(s->reserved_regs, TCG_REG_SP);
>> + tcg_regset_set_reg(s->reserved_regs, TCG_REG_TMP);
>> + tcg_regset_set_reg(s->reserved_regs, TCG_REG_X18); /* platform register
>> */
>
> Reserve the frame pointer.
Ok.
> r~
Claudio
