On 7/3/24 4:01 AM, Kewen.Lin wrote:
>> - if (TARGET_POWER10
>> + if (TARGET_POWER8
>> && info->calls_p
>> && DEFAULT_ABI == ABI_ELFv2
>> && rs6000_rop_protect)
>
> Nit: I noticed that this is the only place to change
> info->rop_hash_size to non-zero, and ...
>
>> @@ -3277,7 +3277,7 @@ rs6000_emit_prologue (void)
>> /* NOTE: The hashst isn't needed if we're going to do a sibcall,
>> but there's no way to know that here. Harmless except for
>> performance, of course. */
>> - if (TARGET_POWER10 && rs6000_rop_protect && info->rop_hash_size != 0)
>> + if (TARGET_POWER8 && rs6000_rop_protect && info->rop_hash_size != 0)
>
> ... this condition and ...
>
>> {
>> gcc_assert (DEFAULT_ABI == ABI_ELFv2);
>> rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM);
>> @@ -5056,7 +5056,7 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type)
>>
>> /* The ROP hash check must occur after the stack pointer is restored
>> (since the hash involves r1), and is not performed for a sibcall. */
>> - if (TARGET_POWER10
>> + if (TARGET_POWER8> && rs6000_rop_protect
>> && info->rop_hash_size != 0
>
> ... here, both check info->rop_hash_size isn't zero, I think we can drop these
> two TARGET_POWER10 (TARGET_POWER8) and rs6000_rop_protect checks? Instead
> just
> update the inner gcc_assert (now checking DEFAULT_ABI == ABI_ELFv2) by extra
> checkings on TARGET_POWER8 && rs6000_rop_protect?
>
> The other looks good to me, ok for trunk with this nit tweaked (if you agree
> with it and re-tested well), thanks!
I agree with you, because the next patch I haven't submitted yet (waiting
on this to get in), makes that simplification as part of the adding earlier
checking of invalid options. :-) The follow-on patch will not only remove
the TARGET_* and the 2nd/3rd rs6000_rop_protect usage, but will also remove
the test and asserts of ELFv2...because we've already verified valid option
usage earlier in the normal options handling code.
Therefore, I'd like to keep this patch as simple as possible and limited to
the TARGET_POWER10 -> TARGET_POWER8 change and the cleanup of those tests is
coming in the next patch...which has already been tested.
Peter
