Re: [PATCH, MIPS] Add support for cins Octeon instruction

[Adam Nemet <anemet at caviumnetworks dot com>]

Richard Sandiford wrote:
> Adam Nemet <anemet@caviumnetworks.com> writes:
>> +/* The canonical form of a mask-low-and-shift-left operation is
>> +   (and (ashift X SHIFT) MASK) where MASK has the lower SHIFT number of
>> +   bits cleared.  Thus we need to shift MASK to the right before
>> +   checking if it is a valid mask value.  If true return the length of
>> +   the mask, otherwise return -1.  */
>> +
>> +int
>> +mask_low_and_shift_len (rtx mask, rtx shift)
>> +{
>> +  return exact_log2 ((UINTVAL (mask) >> INTVAL (shift)) + 1);
>> +}
> 
> Perhaps I'm missing something, but is this really valid for SImode
> when the sum of the mask and shift lengths is 32?  Mightn't we get
> something that isn't properly sign-extended?

I thought about this originally, in fact, this is function f in the
second testcase I am adding.  My thinking was that assuming the current
implementation of and:SI (generates and/andi) this might not be valid
but then the original expression would not have been valid either (in
the machine instruction sense, not in RTL sense).  IOW, if we had:

  (and:SI (ashift:SI X 24) (const_int 0xff000000))

and assuming GCC would generate code without removing the redundant
"and" we would generate incorrect code.  This is because (and:SI X
0xffffffff) would use the "and" instruction which is 64-bit.

Being forced to think about this once more ;) now I can see that the
logic is flawed and instead and:SI should be fixed as well.

I will change this to not match if the sign bit is effected by the
operation.  I can also look at fixing and:SI during stage 3.

> Much more minor, but I've tried to avoid declaration assignments
> in mips.c.  (I know this isn't consistent in gcc as a whole.)
> In this case, IN_RANGE_P is specifically designed to evaluate
> the first argument once, so:
> 
>   return IN_RANGE (mask_low_and_shift_len (mask, shift), 1, maxlen);
> 
> avoids the problem while being simpler.

Sure, no problem.  Can you provide some insight why decl assignments are
 undesired?  I consider them misleading within a loop but they seem to
work intuitively outside of loops.

> I'm worried that:
> 
>> +  return exact_log2 ((UINTVAL (mask) >> INTVAL (shift)) + 1);
> 
> is undefined for "out-of-range" shift values.  There's no requirement
> for rtl code to canonicalise the shift amount; see the any_shift
> insns for what we have to do there.

Sure.

I will resubmitted once we agree on the SI question.

Adam