diff mbox

Fix ubsan expansion of multiplication (PR rtl-optimization/60030)

Message ID 20140204124045.GJ12671@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 4, 2014, 12:40 p.m. UTC 
Hi!

ubsan/overflow-1.c test ICEs on ppc32, because lopart is SImode, and we
attempt to do a DImode left shift on it by 32.
We don't really care about the upper bits, those are all shifted away
anyway, so this patch fixes it by adding a paradoxical SUBREG, which results
in better generated code compared to e.g. adding there a ZERO_EXTEND.

Bootstrapped/regtested on x86_64-linux and i686-linux, additionally tested
with ubsan tests on x86_64 and i686 with the add/sub/mul/neg with overflow
patterns disabled and tested using ppc* cross on overflow-1.c.

Ok for trunk?

2014-02-04  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/60030
	* internal-fn.c (ubsan_expand_si_overflow_mul_check): Surround
	lopart with paradoxical subreg before shifting it up by hprec.


	Jakub

Comments

Richard Henderson Feb. 6, 2014, 2:53 p.m. UTC | #1 
On 02/04/2014 04:40 AM, Jakub Jelinek wrote:
> -	  tem = expand_shift (LSHIFT_EXPR, mode, lopart, hprec, NULL_RTX, 1);
> +	  tem = gen_rtx_SUBREG (mode, lopart, 0);
> +	  tem = expand_shift (LSHIFT_EXPR, mode, tem, hprec, NULL_RTX, 1);

I would be happier with gen_lowpart rather than the explicit gen_rtx_subreg.

Ok with that change.


r~
Jakub Jelinek Feb. 6, 2014, 4:02 p.m. UTC | #2 
On Thu, Feb 06, 2014 at 06:53:55AM -0800, Richard Henderson wrote:
> On 02/04/2014 04:40 AM, Jakub Jelinek wrote:
> > -	  tem = expand_shift (LSHIFT_EXPR, mode, lopart, hprec, NULL_RTX, 1);
> > +	  tem = gen_rtx_SUBREG (mode, lopart, 0);
> > +	  tem = expand_shift (LSHIFT_EXPR, mode, tem, hprec, NULL_RTX, 1);
> 
> I would be happier with gen_lowpart rather than the explicit gen_rtx_subreg.

I need a paradoxical subreg, gen_lowpart ICEs in that case
(for ppc* here lopart is SImode, I need DImode paradoxical subreg of that,
so that I can shift it up by 32 bits and thus get a DImode value
with the lopart bits in the upper 32 bits and zero in the rest.

	Jakub
Richard Henderson Feb. 6, 2014, 4:23 p.m. UTC | #3 
On 02/06/2014 08:02 AM, Jakub Jelinek wrote:
> On Thu, Feb 06, 2014 at 06:53:55AM -0800, Richard Henderson wrote:
>> On 02/04/2014 04:40 AM, Jakub Jelinek wrote:
>>> -	  tem = expand_shift (LSHIFT_EXPR, mode, lopart, hprec, NULL_RTX, 1);
>>> +	  tem = gen_rtx_SUBREG (mode, lopart, 0);
>>> +	  tem = expand_shift (LSHIFT_EXPR, mode, tem, hprec, NULL_RTX, 1);
>>
>> I would be happier with gen_lowpart rather than the explicit gen_rtx_subreg.
> 
> I need a paradoxical subreg, gen_lowpart ICEs in that case

It does?  Since when?  I've certainly used it for paradoxicals in the past.

Oh, I see, yes, it would ICE for a multi-word paradoxical subreg.  But that
sort of thing is ... skirting the bounds of validity at best.

Surely we should be able to optimize away a zero-extension in all cases?


r~
Jakub Jelinek Feb. 6, 2014, 4:25 p.m. UTC | #4 
On Thu, Feb 06, 2014 at 08:23:00AM -0800, Richard Henderson wrote:
> On 02/06/2014 08:02 AM, Jakub Jelinek wrote:
> > On Thu, Feb 06, 2014 at 06:53:55AM -0800, Richard Henderson wrote:
> >> On 02/04/2014 04:40 AM, Jakub Jelinek wrote:
> >>> -	  tem = expand_shift (LSHIFT_EXPR, mode, lopart, hprec, NULL_RTX, 1);
> >>> +	  tem = gen_rtx_SUBREG (mode, lopart, 0);
> >>> +	  tem = expand_shift (LSHIFT_EXPR, mode, tem, hprec, NULL_RTX, 1);
> >>
> >> I would be happier with gen_lowpart rather than the explicit gen_rtx_subreg.
> > 
> > I need a paradoxical subreg, gen_lowpart ICEs in that case
> 
> It does?  Since when?  I've certainly used it for paradoxicals in the past.
> 
> Oh, I see, yes, it would ICE for a multi-word paradoxical subreg.  But that
> sort of thing is ... skirting the bounds of validity at best.
> 
> Surely we should be able to optimize away a zero-extension in all cases?

All I know is that the generated code with the ZERO_EXTEND has been larger
than with the paradoxical subreg.  But if you prefer, I can surely emit a
ZERO_EXTEND and open a PR for GCC 5.0 that we should investigate why we
generate worse code then.

	Jakub
Richard Henderson Feb. 6, 2014, 4:27 p.m. UTC | #5 
On 02/06/2014 08:25 AM, Jakub Jelinek wrote:
> On Thu, Feb 06, 2014 at 08:23:00AM -0800, Richard Henderson wrote:
>> On 02/06/2014 08:02 AM, Jakub Jelinek wrote:
>>> On Thu, Feb 06, 2014 at 06:53:55AM -0800, Richard Henderson wrote:
>>>> On 02/04/2014 04:40 AM, Jakub Jelinek wrote:
>>>>> -	  tem = expand_shift (LSHIFT_EXPR, mode, lopart, hprec, NULL_RTX, 1);
>>>>> +	  tem = gen_rtx_SUBREG (mode, lopart, 0);
>>>>> +	  tem = expand_shift (LSHIFT_EXPR, mode, tem, hprec, NULL_RTX, 1);
>>>>
>>>> I would be happier with gen_lowpart rather than the explicit gen_rtx_subreg.
>>>
>>> I need a paradoxical subreg, gen_lowpart ICEs in that case
>>
>> It does?  Since when?  I've certainly used it for paradoxicals in the past.
>>
>> Oh, I see, yes, it would ICE for a multi-word paradoxical subreg.  But that
>> sort of thing is ... skirting the bounds of validity at best.
>>
>> Surely we should be able to optimize away a zero-extension in all cases?
> 
> All I know is that the generated code with the ZERO_EXTEND has been larger
> than with the paradoxical subreg.  But if you prefer, I can surely emit a
> ZERO_EXTEND and open a PR for GCC 5.0 that we should investigate why we
> generate worse code then.

Yes please.


r~
diff mbox

Patch

--- gcc/internal-fn.c.jj	2014-01-29 12:43:24.000000000 +0100
+++ gcc/internal-fn.c	2014-02-03 10:40:57.000000000 +0100
@@ -646,7 +646,8 @@  ubsan_expand_si_overflow_mul_check (gimp
 	    emit_cmp_and_jump_insns (hipart, const0_rtx, GE, NULL_RTX, hmode,
 				     false, after_hipart_neg, PROB_EVEN);
 
-	  tem = expand_shift (LSHIFT_EXPR, mode, lopart, hprec, NULL_RTX, 1);
+	  tem = gen_rtx_SUBREG (mode, lopart, 0);
+	  tem = expand_shift (LSHIFT_EXPR, mode, tem, hprec, NULL_RTX, 1);
 	  tem = expand_simple_binop (mode, MINUS, loxhi, tem, NULL_RTX,
 				     1, OPTAB_DIRECT);
 	  emit_move_insn (loxhi, tem);