> On Sep 15, 2020, at 4:11 AM, Richard Sandiford <richard.sandif...@arm.com>
> wrote:
>
> Qing Zhao <qing.z...@oracle.com <mailto:qing.z...@oracle.com>> writes:
>>> On Sep 14, 2020, at 2:20 PM, Richard Sandiford <richard.sandif...@arm.com>
>>> wrote:
>>>
>>> Qing Zhao <qing.z...@oracle.com <mailto:qing.z...@oracle.com>> writes:
>>>>> On Sep 14, 2020, at 11:33 AM, Richard Sandiford
>>>>> <richard.sandif...@arm.com> wrote:
>>>>>
>>>>> Qing Zhao <qing.z...@oracle.com> writes:
>>>>>>> Like I mentioned earlier though, passes that run after
>>>>>>> pass_thread_prologue_and_epilogue can use call-clobbered registers that
>>>>>>> weren't previously used. For example, on x86_64, the function might
>>>>>>> not use %r8 when the prologue, epilogue and returns are generated,
>>>>>>> but pass_regrename might later introduce a new use of %r8. AIUI,
>>>>>>> the “used” version of the new command-line option is supposed to clear
>>>>>>> %r8 in these circumstances, but it wouldn't do so if the data was
>>>>>>> collected at the point that the return is generated.
>>>>>>
>>>>>> Thanks for the information.
>>>>>>
>>>>>>>
>>>>>>> That's why I think it's more robust to do this later (at the beginning
>>>>>>> of pass_late_compilation) and insert the zeroing before returns that
>>>>>>> already exist.
>>>>>>
>>>>>> Yes, looks like it’s not correct to insert the zeroing at the time when
>>>>>> prologue, epilogue and return are generated.
>>>>>> As I also checked, “return” might be also generated as late as pass
>>>>>> “pass_delay_slots”, So, shall we move the
>>>>>> New pass as late as possible?
>>>>>
>>>>> If we insert the zeroing before pass_delay_slots and describe the
>>>>> result correctly, pass_delay_slots should do the right thing.
>>>>>
>>>>> Describing the result correctly includes ensuring that the cleared
>>>>> registers are treated as live on exit from the function, so that the
>>>>> zeroing doesn't get deleted again, or skipped by pass_delay_slots.
>>>>
>>>> In the current implementation for x86, when we generating a zeroing insn
>>>> as the following:
>>>>
>>>> (insn 18 16 19 2 (set (reg:SI 1 dx)
>>>> (const_int 0 [0])) "t10.c":11:1 -1
>>>> (nil))
>>>> (insn 19 18 20 2 (unspec_volatile [
>>>> (reg:SI 1 dx)
>>>> ] UNSPECV_PRO_EPILOGUE_USE) "t10.c":11:1 -1
>>>> (nil))
>>>>
>>>> i.e, after each zeroing insn, the register that is zeroed is marked as
>>>> “UNSPECV_PRO_EPILOGUE_USE”,
>>>> By doing this, we can avoid this zeroing insn from being deleted or
>>>> skipped.
>>>>
>>>> Is doing this enough to describe the result correctly?
>>>> Is there other thing we need to do in addition to this?
>>>
>>> I guess that works, but I think it would be better to abstract
>>> EPILOGUE_USES into a new target-independent wrapper function that
>>> (a) returns true if EPILOGUE_USES itself returns true and (b) returns
>>> true for registers that need to be zero on return, if the zeroing
>>> instructions have already been inserted. The places that currently
>>> test EPILOGUE_USES should then test this new wrapper function instead.
>>
>> Okay, I see.
>> Looks like that EPILOGUE_USES is used in df-scan.c to compute the data flow
>> information. If EPILOUGE_USES return true
>> for registers that need to be zeroed on return, those registers will be
>> included in the data flow information, as a result, later
>> passes will not be able to delete them.
>>
>> This sounds to be a cleaner approach than the current one that marks the
>> registers “UNSPECV_PRO_EPILOGUE_USE”.
>>
>> A more detailed implementation question on this:
>> Where should I put this new target-independent wrapper function in? Which
>> header file will be a proper place to hold this new function?
>
> Not a strong opinion, but: maybe df.h and df-scan.c, since this is
> really a DF query.
Okay.
>
>>> After inserting the zeroing instructions, the pass should recompute the
>>> live-out sets based on this.
>
> Sorry, I was wrong here. It should *cause* the sets to be recomputed
> where necessary (rather than recompute them directly), but see below.
>
>> Is only computing the live-out sets of the block that including the return
>> insn enough? Or we should re-compute the whole procedure?
>>
>> Which utility routine I should use to recompute the live-out sets?
>
> Inserting the instructions will cause the containing block to be marked
> dirty, via df_set_bb_dirty. I think the pass should also call
> df_set_bb_dirty on the exit block itself, to indicate that the
> wrapper around EPILOGUE_USES has changed behaviour, but that might
> not be necessary.
>
> This gives the df machinery enough information to work out what has changed.
> It will then propagate those changes throughout the function. (I don't
> think any propagation would be necessary here, but if I'm wrong about that,
> then the df machinery will do whatever propagation is necessary.)
>
> However, the convention is for a pass that uses the df machinery to call
> df_analyze first. This call to df_analyze updates any stale df information.
>
> So unlike what I said yesterday, the pass itself doesn't need to make sure
> that the df information is up-to-date. It just needs to indicate what
> has changed, as above.
>
> In the case of pass_delay_slots, pass_free_cfg has:
>
> /* The resource.c machinery uses DF but the CFG isn't guaranteed to be
> valid at that point so it would be too late to call df_analyze. */
> if (DELAY_SLOTS && optimize > 0 && flag_delayed_branch)
> {
> df_note_add_problem ();
> df_analyze ();
> }
>
> Any other machine-specific passes that use df already need to call
> df_analyze (if they use the df machinery). So simply marking what
> has changed is enough (by design).
So, in this new pass, I need:
1. Call “df_analyze” in the beginning to get the up-to-data df information;
2. After generating the zero insns, mark the containing block with
“df_set_bb_dirty”.
3. mark the exit block with “df_set_bb_dirty” to indicate the wrapper around
EPILOGUE_USES changed
Behavior. (This might not need since “df_analyze” in the next pass will
call EPILOGUE_USES automatically? )
Is the above enough for DF?
(BTW, how expensive to call “df_analyze”?)
>
>> My understanding is:
>>
>> In our new pass that is put in the beginning of the pass_late_compilation,
>> I,e pass_zero_call_used_regs;
>>
>> PUSH_INSERT_PASSES_WITHIN (pass_late_compilation)
>> ++++ NEXT_PASS (pass_zero_call_used_regs);
>> NEXT_PASS (pass_compute_alignments);
>> NEXT_PASS (pass_variable_tracking);
>> NEXT_PASS (pass_free_cfg);
>> NEXT_PASS (pass_machine_reorg);
>> NEXT_PASS (pass_cleanup_barriers);
>> NEXT_PASS (pass_delay_slots);
>>
>> When we scan the EXIT BLOCK of the routine, all the return insns have
>> already been there.
>> The later passes including “pass_delay_slots” will not generate additional
>> returns anymore, they might just call “target.gen_return” or
>> “target.gen_simple_return() to replace
>> “ret_rtx” or “simple_ret_rtx” ?
>
> Kind-of. pass_delay_slots can also duplicate code, so it's not always a
> straight replacement. But the point is that returns don't appear out of
> nowhere. There has to be a semantic reason for them to exist. The
> behaviour of the function after pass_delay_slots has to be the same
> as it was before the pass (disregarding undefined behaviour). Once we've
> added clearing of the zero registers to all return paths, that clearing
> becomes part of the behaviour of the function, and so will be part of
> the behaviour after pass_delay_slots as well.
>
> So I don't think the problem is with passes generating new returns.
> It's more whether they could use new registers that then need to be
> cleared, which is the main justification for running the new pass
> so late in the pipeline.
agreed.
>
> In principle, there's nothing stopping pass_delay_slots allocating
> new registers (like pass_regrename does), and in principle that could
> introduce the need to do more clearing. But I don't think the current
> pass does that. The pass is also very much legacy code at this point,
> so the chances of new optimisations being added to it are fairly low.
> If that did happen, I think it would be reasonable to expect the pass
> to work within the set of registers that have already been allocated,
> at least when your new option is in effect.
Okay, thanks for the information.
Qing
>
> Thanks,
> Richard
