[PATCH[[mips] Add octeon branch instructions bbit0/bbit032/bbit1/bbit132

Daniel Sanders Daniel.Sanders at imgtec.com
Mon Jan 19 07:15:41 PST 2015


Thanks. You've fixed the bulk of the comments but there's one bit left in the test and one new whitespace error in immZExt5_64.

> --- a/test/CodeGen/Mips/octeon.ll
> +++ b/test/CodeGen/Mips/octeon.ll
> @@ -93,3 +93,75 @@ entry:
> <snip>
> +define i64 @bbit032(i64 %a) nounwind {
> +entry:
> +; OCTEON-LABEL: bbit032:
> +; OCTEON: bbit032 $4, 3, $[[BB0:BB[0-9_]+]]
> +; MIPS64-LABEL: bbit032:
> +; MIPS64: daddiu  $1, $zero, 1
> +; MIPS64: dsll    $1, $1, 35
> +; MIPS64: and     $1, $4, $1
> +; MIPS64: beqz    $1, $[[BB0:BB[0-9_]+]]
> +  %bit = and i64 %a, 34359738368
> +  %res = icmp eq i64 %bit, 0
> +  br i1 %res, label %endif, label %if
> +if:
> +  ret i64 48
> +
> +endif:
> +  ret i64 12
> +}

That's very close but you need to use variables for the unpredictable registers. The $4 is predictable since the calling convention requires it to be $4. $zero is also predictable because it's the hardwired zero. However, the $1's could be any allocatable register. Here's the MIPS64 lines but using variables for the unpredictable registers:
  ; MIPS64-LABEL: bbit032:
  ; MIPS64: daddiu  $[[T0:[0-9]+]], $zero, 1
  ; MIPS64: dsll    $[[T1:[0-9]+]], $[[T0]], 35
  ; MIPS64: and     $[[T2:[0-9]+]], $4, $[[T1]]
  ; MIPS64: beqz    $[[T2]], $[[BB0:BB[0-9_]+]]

> --- a/lib/Target/Mips/Mips64InstrInfo.td
> +++ b/lib/Target/Mips/Mips64InstrInfo.td
> @@ -41,6 +45,38 @@ def immSExt10_64 : PatLeaf<(i64 imm),
>  def immZExt16_64 : PatLeaf<(i64 imm),
>                             [{ return isInt<16>(N->getZExtValue()); }]>;
>  
> +def immZExt5_64 : ImmLeaf<i64, [{return Imm == (Imm & 0x1f);}]>;

There should be a space before the 'return' and after the first semicolon.
________________________________________
From: Kai Nacke [kai.nacke at redstar.de]
Sent: 19 January 2015 06:21
To: Daniel Sanders; llvm-commits
Subject: Re: [PATCH[[mips] Add octeon branch instructions bbit0/bbit032/bbit1/bbit132

Hi Daniel!

During testing I found that I put uimm5_64 and immZExt5_64 into another patch. I moved the definition into this patch to fix the compile problem.

Regards,
Kai

On 14.01.2015 11:50, Kai Nacke wrote:
> Hi Daniel!
>
> I tried to address all your comments but I am unsure if I really fixed
> the formatting issue. Please have a look again at it. Thanks.
> I also realized that I can provide the constant as a parameter. This
> makes the patch smaller and clearer IMHO.
>
> Currently I focus on the i64 types. I will look at the smaller later.
> I think that I need this functions for cins/exts, too.
>
> I cleanup the rest of the test cases in a later patch (adding multiple
> --check-prefixes etc.)
>
> Regards,
> Kai
>
> On 12.01.2015 11:59, Daniel Sanders wrote:
>> Hi Kai,
>>
>> LGTM with a couple nits:
>> * The indentation on the lines after each of the
>> CBranchImm/CBranchImm32 def is incorrect.
>> * Could you rename CBranchImm? The current name implies it's more
>> generic than the pattern really is. CBranchBitNum is one possibility I
>> can think of.
>> * In the testcase, could you fill out the registers in the MIPS64 case
>> too? You can use FileCheck variables for the ones that are unpredictable.
>>
>> Not needed for this patch since the addi64 and mul tests do the same
>> thing but it would be good to use multiple -check-prefixes so that we
>> can just specify 'ALL-LABEL: bbit0:' once rather than for each case.
>>
>> Also a question:
>> BBIT0, BBIT1, and PowerOf2LO look like they'd be fine for i32 and
>> smaller too. Do you intend to implement the char/short/int cases later?
>>
>>> -----Original Message-----
>>> From: Kai Nacke [mailto:kai.nacke at redstar.de]
>>> Sent: 11 January 2015 18:07
>>> To: llvm-commits; Daniel Sanders
>>> Subject: Re: [PATCH[[mips] Add octeon branch instructions
>>> bbit0/bbit032/bbit1/bbit132
>>>
>>> Hi Daniel!
>>>
>>> I refined the patch a bit: there was a bug in the instruction pattern.
>>> I also added new patterns with the result that the instructions are now
>>> selected for C code like
>>>
>>> if (a & 0x08) { .. }
>>>
>>> Test cases are included.
>>>
>>> Please review.
>>>
>>> Regards,
>>> Kai
>>>
>>> On 06.01.2015 07:12, Kai Nacke wrote:
>>>> Hi Daniel,
>>>>
>>>> the attached patch adds the octeon branch instructions
>>>> bbit0/bbit032/bbit1/bbit132. Test case is included. Please review.
>>>>
>>>> BTW: I did not implement the automatic change of the instruction if the
>>>> constant does not fit (e.g. bbit0 $22, 42, foo -> bbit032 $22, 10,
>>>> foo).
>>>> Do you have an suggestion where to implement this? Thanks.
>>>>
>>>> Regards,
>>>> Kai
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>
>>
>
>
>
> _______________________________________________ llvm-commits mailing
> list llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>





More information about the llvm-commits mailing list