Re: [PATCH] PowerPC: Implement TARGET_ATOMIC_ASSIGN_EXPAND_FENV

[David Edelsohn <dje dot gcc at gmail dot com>]

Hi, Adhemerval

I cornered Honza during his visit to IBM Research to help me
understand my concerns with the function.

The code for *hold does:

+  tree fenv_var = create_tmp_var (double_type_node, NULL);
+
+  tree hold_mffs = build2 (MODIFY_EXPR, void_type_node, fenv_var, call_mffs);
+
+  tree fenv_llu = build1 (VIEW_CONVERT_EXPR, uint64_type_node, fenv_var);

The code for *clear does:

+  tree fenv_clear = create_tmp_var (double_type_node, NULL);
+
+  tree clear_mffs = build2 (MODIFY_EXPR, void_type_node, fenv_clear,
call_mffs);
+
+  tree fenv_clean_llu = build1 (VIEW_CONVERT_EXPR, uint64_type_node, fenv_var);

Note that *clear creates fenv_clear, but uses fenv_var that was
created earlier for *hold (probably should have been fenv_hold) or
something to distinguish it.

The code for *update does:

+  tree old_fenv = create_tmp_var (double_type_node, NULL);
+  tree update_mffs = build2 (MODIFY_EXPR, void_type_node, old_fenv, call_mffs);
+
+  tree old_llu = build1 (VIEW_CONVERT_EXPR, uint64_type_node, update_mffs);

But it never actually uses old_fenv that it obtained from the call to
call_mffs().

The current implementation is trying to follow the punning and
conversions in the C code, but, according to Honza, it does not need
to.  It should not need the temporary variable nor the MODIFY_EXPR.
The implementation of *update shows that the temporary really is not
necessary because it (accidentally) is not referenced and not used.

The code for *hold and *clear should be converted to the style of
*update, without the temporary. The later instruction selection and
register allocation in GCC should handle the conversion between
double_type and uint64_type through the VIEW_CONVERT_EXPR without an
explicit temporary. The call to mffs can be inserted directly into the
rest of the tree being built (creating a large and ugly tree).

Then the final COMPOUND_EXPR is not needed in any of the cases, as it
already is omitted in the *update case. The accidental omission of a
reference to old_fenv is what allowed it to be omitted from the
*update case, which prompted my original question.

Thanks, David