[PATCH v2 08/31] jump: Also handle jumps wrapped in UNSPEC or UNSPEC_VOLATILE
Jeff Law
law@redhat.com
Thu Dec 3 22:20:11 GMT 2020
On 12/2/20 8:50 PM, Maciej W. Rozycki wrote:
> VAX has interlocked branch instructions used for atomic operations and
> we want to have them wrapped in UNSPEC_VOLATILE so as not to have code
> carried across. This however breaks with jump optimization and leads
> to an ICE in the build of libbacktrace like:
>
> .../libbacktrace/mmap.c:190:1: internal compiler error: in fixup_reorder_chain, at cfgrtl.c:3934
> 190 | }
> | ^
> 0x1087d46b fixup_reorder_chain
> .../gcc/cfgrtl.c:3934
> 0x1087f29f cfg_layout_finalize()
> .../gcc/cfgrtl.c:4447
> 0x1087c74f execute
> .../gcc/cfgrtl.c:3662
>
> on RTL like:
>
> (jump_insn 18 17 150 4 (unspec_volatile [
> (set (pc)
> (if_then_else (eq (zero_extract:SI (mem/v:SI (reg/f:SI 23 [ _2 ]) [-1 S4 A32])
> (const_int 1 [0x1])
> (const_int 0 [0]))
> (const_int 1 [0x1]))
> (label_ref 20)
> (pc)))
> (set (zero_extract:SI (mem/v:SI (reg/f:SI 23 [ _2 ]) [-1 S4 A32])
> (const_int 1 [0x1])
> (const_int 0 [0]))
> (const_int 1 [0x1]))
> ] 101) ".../libbacktrace/mmap.c":135:14 158 {jbbssisi}
> (nil)
> -> 20)
>
> when those branches are enabled with a follow-up change. Also showing
> with:
>
> FAIL: gcc.dg/pr61756.c (internal compiler error)
>
> Handle branches wrapped in UNSPEC_VOLATILE then and, for consistency,
> also in UNSPEC. The presence of UNSPEC_VOLATILE will prevent such
> branches from being removed as they won't be accepted by `onlyjump_p',
> we just need to let them through.
>
> gcc/
> * jump.c (pc_set): Also accept a jump wrapped in UNSPEC or
> UNSPEC_VOLATILE.
> (any_uncondjump_p, any_condjump_p): Update comment accordingly.
> ---
> On Fri, 20 Nov 2020, Jeff Law wrote:
>
>>> Handle branches wrapped in UNSPEC_VOLATILE then and, for consistency,
>>> also in UNSPEC. The presence of UNSPEC_VOLATILE will prevent such
>>> branches from being removed as they won't be accepted by `onlyjump_p',
>>> we just need to let them through.
>>>
>>> gcc/
>>> * jump.c (pc_set): Also accept a jump wrapped in UNSPEC or
>>> UNSPEC_VOLATILE.
>>> (any_uncondjump_p, any_condjump_p): Update comment accordingly.
>> I've got some concerns that there may be users of pc_set where handling
>> UNSPECs would be undesirable. For example the uses in cfgcleanup.
> I've gone through the use of `pc_set' and `any_condjump_p' in detail
> then. Mind that I wouldn't claim expertise with this stuff, just common
> sense backed with documentation and source code available.
>
> It appears safe to me though, as the really dangerous cases where a jump
> is to be removed, in `thread_jump' and `outgoing_edges_match', are guarded
> with calls to `onlyjump_p' (the reference to which from the description of
> `any_condjump_p' making a promise it will guard the relevant cases is what
> made me pretty confident with my change being technically correct), which
> will reject any jumps wrapped into UNSPEC_VOLATILE or even UNSPEC (though
> arguably the latter case might be an overkill, and we could loosen that
> restriction) on the premise of failing `single_set', which only accepts a
> SET, possibly wrapped into a PARALLEL.
>
> Similarly with branch deletion in `cfg_layout_redirect_edge_and_branch'
> and attempted transformations using `cond_exec_get_condition'.
>
> Those `pc_set' calls that you mention are then only used once the
> containing code has been qualified with `onlyjump_p', so they won't be
> ever called with branches wrapped into UNSPEC_VOLATILE. Likewise the
> `pc_set' calls in `cprop_jump' and `bypass_block'.
>
> The calls in `try_optimize_cfg' trying to convert a conditional branch
> into a corresponding conditional return (necessary to analyse of course
> though not relevant for the VAX; oh, how I long for the Z80) both rely on
> `redirect_jump' and possibly `invert_jump', and they're supposed to fail
> if a suitably modified original rtx cannot be matched with any RTL pattern
> (though AFAICS `redirect_exp_1' will just fail due to the lack of explicit
> UNSPEC_VOLATILE/UNSPEC support and so will `invert_jump' as it relies on
> it too).
>
> Except for one case the use in icvt appear safe to me: all legs except
> for ones calling into `dead_or_predicable' refer to `onlyjump_p'. Then in
> `dead_or_predicable' we have two cases, NCE and CE. The NCE one there is
> safe due to `can_move_insns_across' rejecting the move in the case of any
> UNSPEC_VOLATILE insn in either block. The CE one isn't because ultimately
> it will delete the jump without validating it with `onlyjump_p' AFAICT.
>
> This will not affect VAX as it doesn't have conditional execution, and is
> not a fault in my change. I think it needs fixing though, and I will post
> a patch separately, along with a minor clean-up in this area.
>
> In `force_nonfallthru_and_redirect' we have `gcc_assert (redirected)' and
> this may well trigger sometime. Perhaps we need to make `redirect_jump'
> understand UNSPEC_VOLATILE/UNSPEC? WDYT?
I'd lean against this. In theory, we're just talking about redirecting
a branch. But targets tend to add the UNSPECs when there's something
going on that we can't really expose and there isn't a good way to tell
the generic bits that direction is, or is not OK.
>
> Finally I feel like we ought to avoid loop unrolling with the relevant
> jumps as pasting the body of a loop multiple times within is semantically
> equivalent to jump deletion (between the iterations unrolled) even though
> we do not physically delete the RTL insn, so it needs to be guarded with
> `onlyjump_p', and to me it does not appear to. I'll post a proposed
> change along with the other ones in a small series for easier reference.
I'd tend to agree.
> ould it make sense to have the handling of UNSPECs be conditional so
>> that we don't get unexpected jump threading, cross jumping, etc?
> So for the sake of this consideration I actually made such a change (for
> `pc_set' and `any_condjump_p'), applying the extra condition mechanically
> throughut our code and making it `true' for the equivalent of the original
> change right away where it was obvious to me it did not matter. Then I
> went through the remaining ~50 cases in detail flipping the condition to
> `true' as I ticked off cases found harmless and finally came up with the
> conclusion above. If any new dangerous cases arise, then I think they
> need to be guarded as appropriate, with `onlyjump_p' for deletions and
> `can_move_insns_across' for reordering.
>
> My overall view is with jumps wrapped into plain UNSPECs the middle end
> ought to be allowed to do whatever it wishes, it's an annotation really if
> the rtx within can be usefully interpreted. While with UNSPEC_VOLATILEs
> obviously the usual rules for a barrier apply, i.e. the jump must not be
> deleted or code carried across it. I think however that the target label
> is allowed to be redirected if we can prove (which I believe we do) that
> the resulting code is semantically equivalent, e.g. a conditional jump to
> an unconditional one. And the condition is allowed to be reversed as long
> as, obviously, the backend has provided a definition for a corresponding
> reverse UNSPEC_VOLATILE branch. Physical duplication (cf. `rotate_loop')
> is allowed too.
The counter case would be a port that inserts its own long branch
handling stubs and then might use UNSPECs to prevent the optimizers from
scrambling things again. I'm not aware of this actually happening in
our sources, but it's the first case that comes to mind.
>
> I'll post the six patches mentioned shortly once I have written their
> descriptions. Here's an update to 08/31 with a clarification WRT
> `onlyjump_p' for `any_uncondjump_p'; no functional change.
>
> Maciej
>
> Changes from v1:
>
> - Update `any_uncondjump_p' description.
> ---
> gcc/jump.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
OK
jeff
More information about the Gcc-patches
mailing list