Re: [PATCH] Fix PR target/28946

[Uros Bizjak <ubizjak at gmail dot com>]

H. J. Lu wrote:

> On Tue, Sep 05, 2006 at 07:20:02AM -0600, Roger Sayle wrote:
>  
>
> > On Tue, 5 Sep 2006, Uros Bizjak wrote:
> >    
> >
> > > 2006-09-06 Uros Bizjak <uros@kss-loka.si>
> > >
> > > 	PR target/28946
> > > 	* combine.c (try_combine): Force PARALLEL of comparison and
> > > 	arithmetic insn even if arithmetic result is not used.
> > >
> > >      * gcc.target/i386/pr28946.c: New test.
> > >      
> > I was going to point out tht a generic change to combine like this
> > really needs more testing that C & C++ on x86, especially during
> > stage 3.  However, from your latest comments in the bugzilla PR it
> >    
>
> As I have pointed out in the bug report, some recent processors need
> the extra "testl   %eax, %eax" here
>
>       shrl    $5, %eax
>       testl   %eax, %eax
>
> to avoid partial flag register stall since a shift instruction may
> not set flag register since shift count may be 0.
>
>  
According to the comment in i386.md, shift by zero problem is addressed 
this way:

;; This pattern can't accept a variable shift count, since shifts by
;; zero don't affect the flags.  We assume that shifts by constant
;; zero are optimized away.
(define_insn "*ashldi3_cmp_rex64"
 [(set (reg FLAGS_REG)
   (compare
     (ashift:DI (match_operand:DI 1 "nonimmediate_operand" "0")
            (match_operand:QI 2 "immediate_operand" "e"))
     (const_int 0)))
  (set (match_operand:DI 0 "nonimmediate_operand" "=rm")
   (ashift:DI (match_dup 1) (match_dup 2)))]
 "TARGET_64BIT && ix86_match_ccmode (insn, CCGOCmode)
  && ix86_binary_operator_ok (ASHIFT, DImode, operands)"

However, as suggested by others (Rask Ingemann and Roger), perhaps 
proposed combine patch was too big gun for a problem. I'll try to solve 
the problem by introducing new RTL patterns. These new patterns can be 
disabled for certain "problematic" targets, so it also addresses your 
concerns.

Uros.