Re: [PATCH][4.1/4.2] Fix PR27260, fallout of fix for PR27095

[Roger Sayle <roger at eyesopen dot com>]

Hi Alan,

On Wed, 26 Apr 2006, Alan Modra wrote:
> :ADDPATCH middle-end:
>
> 	PR middle-end/27260
> 	* builtins.c (expand_builtin_memset): Don't use SAVE_EXPR on args
> 	or expand libcall here.  Instead, just throw away rtl on failure.

Sorry for the delay in commenting, I've been slowly working through
my backlog, and the subject line gave no immediate clue this issue
was middle-end related.

Perhaps its a sign of my age, but I'm cautious about removing the
builtin_save_expr calls from builtins.c.  You might be right that
with the current tree-ssa infrastructure SAVE_EXPRs are much rarer,
but I worry that the process of RTL expansion and the recursive
nature of builtin expansion may introduce SAVE_EXPRs that we didn't
anticipate.

Likewise, I also find the behaviour of SAVE_EXPR RTL expansion bizarre,
where the context in which a tree is evaluated affects the value that
is saved!?  But I agree with whoever commented that there may be a
lot of code that depends upon this non-intuitive semantics.

My personal recommendation would be to take the conservative approach.
Remove the "fold_build1 (CONVERT_EXPR, unsigned_char_type_node, ...)"
call from expand_builtin_memset, and instead call expand_expr with its
native (integer) mode.  This avoids perturbing the SAVE_EXPR to cache
the QImode result.  Instead, at the RTL level call convert_to_mode on
the result.

i.e.

-         tree cval;
          rtx val_rtx;

-         cval = fold_build1 (CONVERT_EXPR, unsigned_char_type_node, val);
          val_rtx = expand_normal (cval);
---
          rtx val_rtx = expand_normal (val);
	  val_rtx = convert_to_mode (QImode, val_rtx, 1);

I'm not sure if its safe to use QImode directly here, perhaps its
better to use TYPE_MODE (unsigned_char_type_node) instead?  Yep,
that idiom is used a few lines further down.


Anyway this solution would (i) avoid removing SAVE_EXPR support from
expand_builtin_memset and (ii) avoid "fixing" the semantics of SAVE_EXPR.
Both of these apsects seem good for a patch to be applied to a release
branch.

Let me know what you think.  Sorry again for not commenting sooner!

Roger
--