Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

["J.W. Jagersma" <jwjagersma@gmail.com>]

On 2020-11-19 06:55, Jeff Law wrote:
> 
> 
> On 11/15/20 6:04 AM, J.W. Jagersma via Gcc-patches wrote:
>> On 2020-11-13 09:41, Richard Biener wrote:
>>> On Thu, Mar 12, 2020 at 1:41 AM J.W. Jagersma via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
>>>> index 2a409dcaffe..58b16aa763a 100644
>>>> --- a/gcc/tree-eh.c
>>>> +++ b/gcc/tree-eh.c
>>>> @@ -2077,6 +2077,8 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>>>             DECL_GIMPLE_REG_P (tmp) = 1;
>>>>           gsi_insert_after (gsi, s, GSI_SAME_STMT);
>>>>         }
>>>> +
>>>> +record_throwing_stmt:
>>>>        /* Look for things that can throw exceptions, and record them.  */
>>>>        if (state->cur_region && stmt_could_throw_p (cfun, stmt))
>>>>         {
>>>> @@ -2085,6 +2087,36 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>>>         }
>>>>        break;
>>>>
>>>> +    case GIMPLE_ASM:
>>>> +      {
>>>> +       /* As above with GIMPLE_ASSIGN.  Change each register output operand
>>>> +          to a temporary and insert a new stmt to assign this to the original
>>>> +          operand.  */
>>>> +       gasm *asm_stmt = as_a <gasm *> (stmt);
>>>> +       if (stmt_could_throw_p (cfun, stmt)
>>>> +           && gimple_asm_noutputs (asm_stmt) > 0
>>>> +           && gimple_stmt_may_fallthru (stmt))
>>>> +         {
>>>> +           for (unsigned i = 0; i < gimple_asm_noutputs (asm_stmt); ++i)
>>>> +             {
>>>> +               tree op = gimple_asm_output_op (asm_stmt, i);
>>>> +               tree opval = TREE_VALUE (op);
>>>> +               if (tree_could_throw_p (opval)
>>>> +                   || !is_gimple_reg_type (TREE_TYPE (opval))
>>>> +                   || !is_gimple_reg (get_base_address (opval)))
>>>> +                 continue;
>>>> +
>>>> +               tree tmp = create_tmp_reg (TREE_TYPE (opval));
>>>> +               gimple *s = gimple_build_assign (opval, tmp);
>>>> +               gimple_set_location (s, gimple_location (stmt));
>>>> +               gimple_set_block (s, gimple_block (stmt));
>>>> +               TREE_VALUE (op) = tmp;
>>>> +               gsi_insert_after (gsi, s, GSI_SAME_STMT);
>>>> +             }
>>>> +         }
>>>> +      }
>>>> +      goto record_throwing_stmt;
>>> Can you avoid the ugly goto by simply duplicating the common code please?
>>>
>>> Otherwise OK.
>>>
>>> As you say volatile asms are already considered throwing in some pieces of
>>> code so this is a step towards fulfilling that promise.
>>>
>>> Thanks,
>>> Richard.
>>>
>> Hi Richard,
>>
>> Thanks for your feedback.  I'll have to check again if I made any other
>> changes since this.  If not, and if there are no further objections, I will
>> resubmit this patch soon with this goto removed.
> Sounds good.  I'll keep an eye out for it.  I think we'll want to look
> at the doc text one more time too to make sure it matches the semantics
> we can actually guarantee.
> 
> Jeff

The only hard guarantees we can make with this patch as-is, is for "=r" and
"+r" operands (clobber), and "+m" (keep).  I suppose the test case for memory
operands should be altered to use '+' and the inverse should be tested too
(what happens when it doesn't throw).

For "=rm" and "=m" we could say that the output value is undefined on
exception, as the first depends on whether the operand is a GIMPLE register
type or not, and the latter may have earlier assignments optimized out.  And
that is valid too, as there may be cases (possibly most cases) where you
wouldn't care what the value is either before or after the asm, if an
exception is thrown.

Another idea I had is to introduce a new operand modifier, eg. '-', which
would signify that the output *must* be considered clobbered on exception,
and it would be an error if a copy is not possible.  Then the meaning of '+'
can be extended to say that the output must be valid regardless of whether an
exception occured or not.  Modifier '=' would mean the same as '+' with the
additional implication that it is okay to eliminate earlier assignments, so
that its value on the EH edge would be undefined.