Qing Zhao <qing.z...@oracle.com> writes:
>>> diff --git a/gcc/flag-types.h b/gcc/flag-types.h
>>> index 852ea76..0f7e503 100644
>>> --- a/gcc/flag-types.h
>>> +++ b/gcc/flag-types.h
>>> @@ -285,6 +285,15 @@ enum sanitize_code {
>>> | SANITIZE_BOUNDS_STRICT
>>> };
>>>
>>> +enum zero_call_used_regs_code {
>>> + UNSET = 0,
>>> + SKIP = 1UL << 0,
>>> + ONLY_USED = 1UL << 1,
>>> + ONLY_GPR = 1UL << 2,
>>> + ONLY_ARG = 1UL << 3,
>>> + ALL = 1UL << 4
>>> +};
>>
>> I'd suggested these names on the assumption that we'd be using
>> a C++ enum class, so that the enum would be referenced as
>> name::ALL, name::SKIP, etc. But I guess using a C++ enum class
>> doesn't work well with bitfields after all.
>>
>> These names are too generic without the name:: scoping though.
>> Perhaps we should put them in a namespace:
>>
>> namespace zero_regs_flags {
>> const unsigned int UNSET = 0;
>> …etc…
>> }
>>
>> (call-used probably doesn't need to be part of the flag names,
>> since the concept is more general than that and call-usedness
>> is really a filter that's being applied on top. Although I guess
>> the same is true of “zero”. ;-))
>>
>> I don't think we should have ALL as a separate flag: ALL is the absence
>> of ONLY_*. Maybe we should have an ENABLED flag that all non-skip
>> combinations use?
>>
>> If it makes things easier, I think it would be good to have e.g.:
>>
>> unsigned int USED_GPR = ENABLED | ONLY_USED | ONLY_GPR;
>>
>> inside the namespace, to reduce the verbosity in the option table.
>
> Then, the final namespace will look like:
>
> namespace zero_regs_flags {
> const unsigned int UNSET = 0;
> const unsigned int SKIP = 1UL << 0;
> const unsigned int ONLY_USED = 1UL << 1;
> const unsigned int ONLY_GPR = 1UL << 2;
> const unsigned int ONLY_ARG = 1UL << 3;
> const unsigned int ENABLED = 1UL << 4;
> const unsigned int USED_GPR_ARG = ONLY_USED | ONLY_GPR | ONLY_ARG;
“ENABLED |” here
> const unsigned int USED_GPR = ENABLED | ONLY_USED | ONLY_GPR;
> const unsigned int USED_ARG = ENABLED | ONLY_USED | ONLY_ARG;
> const unsigned int USED = ENABLED | ONLY_USED;
> const unsigned int ALL_GRP_ARG = ENABLED | ONLY_GPR | ONLY_ARG;
GPR
> const unsigned int ALL_GPR = ENABLED | ONLY_GPR;
> const unsigned int ALL_ARG = ENABLED | ONLY_ARG;
> const unsigned int ALL = ENABLED;
> }
>
> ??
Yeah, looks right modulo the above.
>>> + and 3. it is not live at the return of the routine;
>>> + and 4. it is general registor if gpr_only is true;
>>> + and 5. it is used in the routine if used_only is true;
>>> + and 6. it is a register that passes parameter if arg_only is true;
>>> + */
>>
>> Under GCC formatting, the “and” lines need to be indented under “For each”.
>> Maybe indent the “1.” line a bit more if you think it looks nicer with the
>> numbers lined up (it probably does).
>>
>> Similarly, the last bit of text should end with “. */”, rather than
>> with the “;\n */” above.
>>
>> (Sorry that the rules are so picky about this.)
>
> /* For each of the hard registers, check to see whether we should zero it
> if:
> 1. it is a call-used-registers;
> and 2. it is not a fixed-registers;
> and 3. it is not live at the return of the routine;
> and 4. it is general registor if gpr_only is true;
> and 5. it is used in the routine if used_only is true;
> and 6. it is a register that passes parameter if arg_only is true. */
>
> How about this?
The 1. line looks overindented now :-) Was expecting it to line up
with "2.".
Otherwise looks good.
>>> + HARD_REG_SET zeroed_hardregs;
>>> + start_sequence ();
>>> + zeroed_hardregs = targetm.calls.zero_call_used_regs
>>> (need_zeroed_hardregs);
>>> + rtx_insn *seq = get_insns ();
>>> + end_sequence ();
>>> + if (seq)
>>> + {
>>> + /* Emit the memory blockage and register clobber asm volatile before
>>> + the whole sequence. */
>>> + start_sequence ();
>>> + expand_asm_reg_clobber_mem_blockage (zeroed_hardregs);
>>> + rtx_insn *seq_barrier = get_insns ();
>>> + end_sequence ();
>>> +
>>> + emit_insn_before (seq_barrier, ret);
>>> + emit_insn_before (seq, ret);
>>> +
>>> + /* Update the data flow information. */
>>> + crtl->must_be_zero_on_return |= zeroed_hardregs;
>>> + df_set_bb_dirty (EXIT_BLOCK_PTR_FOR_FN (cfun));
>>> + }
>>> +}
>>> +
>>> +
>>> /* Return a sequence to be used as the epilogue for the current function,
>>> or NULL. */
>>>
>>> @@ -6486,7 +6584,120 @@ make_pass_thread_prologue_and_epilogue
>>> (gcc::context *ctxt)
>>> {
>>> return new pass_thread_prologue_and_epilogue (ctxt);
>>> }
>>> -
>>>
>>> +
>>> +static unsigned int
>>> +rest_of_zero_call_used_regs (void)
>>
>> This needs a function comment. Maybe:
>>
>> /* Iterate over the function's return instructions and insert any
>> register zeroing required by the -fzero-call-used-regs command-line
>> option or the "zero_call_used_regs" function attribute. */
>>
>> Also, we might as well make it:
>>
>> pass_zero_call_used_regs::execute
>>
>> rather than a separate function. The “rest_of_…” stuff is mostly legacy.
>
> You mean to delete the “rest_of_zero_call_used_regs” function, and move its
> body to
> Pass_zero_call_used_regs::execute?
Yes.
>>> +
>>> + crtl->zero_call_used_regs = zero_regs_type;
>>> +
>>> + /* No need to zero call-used-regs when no user request is present. */
>>> + return zero_regs_type > SKIP;
>>
>> Think testing for skip using & SKIP or ==/!= SKIP is more obvious.
>
> Testing with & SKIP or ==/!= SKIP will not work if the flag is UNSET.
But can that happen? I would have expected the command line to be
!= UNDEF at this stage.
If that's not true, then & ENABLED would also work.
>> The i386 tests are Uros's domain, but I think it would be good to have
>> generic tests for all the variants. E.g.:
>>
>> (1) one test per -fzero-call-used-regs option (including skip)
>> (2) one test that tries all valid attribute values (including skip),
>> compiled without -fzero-call-used-regs
>> (3) one test that #includes (2) but is compiled with an arbitrarily-chosen
>> -fzero-call-used-regs (say =all).
>> (4) one test that tries invalid uses of the attribute, e.g.:
>> - one use of the attribute on a variable
>> - one use of the attribute on a function, but with an obviously-wrong
>> value
>> - one use of the attribute on a function, but with -gpr and -arg the
>> wrong way around
>
> You mean to add the above new testing cases to gcc/testsuite/c-c++-common
> For all targets?
Yes.
> Then, we cannot test for the assembly matching, we can only testing for
> “dg-do run” right?
Right. This is in addition to target-specific tests rather than a
replacement for them.
Thanks,
Richard
