on 2024/7/3 23:05, Peter Bergner wrote: > 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.
Looking forward to the upcoming patch, then this patch is ok for trunk, thanks! BR, Kewen