Re: [PATCH, MIPS] Add bbit* Octeon instructions

[Richard Sandiford <rdsandiford@googlemail.com>]

Adam Nemet <anemet@caviumnetworks.com> writes:
> Richard Sandiford wrote:
>> Adam Nemet <anemet@caviumnetworks.com> writes:
>>> If that's acceptable I'd like to sort out whether we need the SI-mode
>>> extraction while fixing this miscompilation.  And then depending on the
>>> outcome also remove SI mode from the bbit pattern.
>> 
>> Won't you still need SImode for 32-bit targets?  (I realise you're
>> planning on removing the o32 multilib, but still... it seems odd
>> remove SImode stuff from 64-bit targets.)
>
> What I meant was to remove the SI pattern for TARGET_64BIT not for
> !TARGET_64BIT (o32).  Basically writing these patterns with a new mode
> iterator:
>
>   (define_mode_iterator WORD [(SI "!TARGET_64BIT") (DI "TARGET_64BIT)])
>
> (In fact I had a parallel version of this patch written and tested with
> WORD and the bbit tests were passing with o32 and I had the same number
> of bbit instructions in libgcj.so.)

OK, but it still seems odd to remove SImode stuff from 64-bit targets.
They generally ought to allow both.  The concern isn't so much the number
of bbit instructions, but the number of mode changes (and the effect they
can have on optimisation).

The target can logically do both 32-bit and 64-bit ops, so I think the
fallback position is to keep things simple and stick to :GPR.

>> Can't you simply use:
>> 
>>   (set_attr "branch_likely" "no")
>> 
>> avoiding the extra define_delay and "has_likely" attribute?
>
> No and I should have really explained that in the email because this had
> been my first reaction as well.  The annul predicates test the
> *candidate instruction* for the delay slot and *not* the branch
> instruction.  Only the actual define_delay predicate is a test on the
> branch instruction so the check needs to go there.

Ah, thanks for the explanation.  But "branch_likely" isn't conceptually
a property of the delay slot insn, so I still think it makes sense to
reuse it.  I.e. rather than:

>  (define_delay (and (eq_attr "type" "branch")
> -		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0)))
> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> +			(eq_attr "has_likely" "yes")))
>    [(eq_attr "can_delay" "yes")
>     (nil)
>     (and (eq_attr "branch_likely" "yes")
>  	(eq_attr "can_delay" "yes"))])
>  
> +;; Branches that don't have likely variants do not annul on false.
> +(define_delay (and (eq_attr "type" "branch")
> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> +			(eq_attr "has_likely" "no")))
> +  [(eq_attr "can_delay" "yes")
> +   (nil)
> +   (nil)])
> +
>  (define_delay (eq_attr "type" "jump")
>    [(eq_attr "can_delay" "yes")
>     (nil)

do:

(define_delay (and (eq_attr "type" "branch")
		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
		   (eq_attr "branch_likely" "yes"))
  [(eq_attr "can_delay" "yes")
   (nil)
   (eq_attr "can_delay" "yes")])

;; Branches that don't have likely variants do not annul on false.
(define_delay (and (eq_attr "type" "branch")
		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
		   (eq_attr "branch_likely" "no"))
  [(eq_attr "can_delay" "yes")
   (nil)
   (nil)])

(I believe "and" can take more than 2 arguments in this context.)

Untested. ;)

OK with that change, if it works.

Richard