diff mbox series

LoongArch: Correct the definition of is_branch_ins()

Message ID 1671006659-23859-1-git-send-email-yangtiezhu@loongson.cn
State New
Headers
Series LoongArch: Correct the definition of is_branch_ins() |

Commit Message

Tiezhu Yang Dec. 14, 2022, 8:30 a.m. UTC 
  The current definition of is_branch_ins() is not correct, it was
first introduced in commit 49aef111e2da ("LoongArch: Add prologue
unwinder support"), in fact, there exist three branch instruction
formats rather than only reg1i21_format.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/include/asm/inst.h | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

Comments

Jinyang He Dec. 16, 2022, 3:18 a.m. UTC | #1 
Hi, Tiezhu,


On 2022-12-14 16:30, Tiezhu Yang wrote:
> The current definition of is_branch_ins() is not correct,

But the branch instruction opcode only use the high 6 bits, [1]. That's 
meaningless because it is the same as the original result. If we care 
too much about the details of instruction format, we will lose a lot of 
tricks on LoongArch instruction coding.

[1] 
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#table-of-instruction-encoding


> it was
> first introduced in commit 49aef111e2da ("LoongArch: Add prologue
> unwinder support"), in fact, there exist three branch instruction
> formats rather than only reg1i21_format.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   arch/loongarch/include/asm/inst.h | 27 +++++++++++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
> index c00e151..42be39be 100644
> --- a/arch/loongarch/include/asm/inst.h
> +++ b/arch/loongarch/include/asm/inst.h
> @@ -329,8 +329,31 @@ static inline bool is_pc_ins(union loongarch_instruction *ip)
>   
>   static inline bool is_branch_ins(union loongarch_instruction *ip)
>   {
> -	return ip->reg1i21_format.opcode >= beqz_op &&
> -		ip->reg1i21_format.opcode <= bgeu_op;
> +	switch (ip->reg0i26_format.opcode) {
> +	case b_op:
> +	case bl_op:
> +		return true;
> +	}
> +
> +	switch (ip->reg1i21_format.opcode) {
> +	case beqz_op:
> +	case bnez_op:
> +	case bceqz_op:
> +		return true;
> +	}
> +
> +	switch (ip->reg2i16_format.opcode) {
> +	case jirl_op:
> +	case beq_op:
> +	case bne_op:
> +	case blt_op:
> +	case bge_op:
> +	case bltu_op:
> +	case bgeu_op:
> +		return true;
> +	}
> +
> +	return false;
>   }
>   
>   static inline bool is_ra_save_ins(union loongarch_instruction *ip)
Tiezhu Yang Dec. 16, 2022, 6:11 a.m. UTC | #2 
On 12/16/2022 11:18 AM, Jinyang He wrote:
> Hi, Tiezhu,
>
>
> On 2022-12-14 16:30, Tiezhu Yang wrote:
>> The current definition of is_branch_ins() is not correct,
>
> But the branch instruction opcode only use the high 6 bits,

Yes, I noticed that, the logic result of current code is right,
but it seems a little strange (only consider reg1i21_format)
at the first glance, the initial aim of this patch is to make
it theoretically correct, maybe it is not the best change.

I think we can neglect the instruction formats and check the
high 6 bits instead, what do you think of the following change?

diff --git a/arch/loongarch/include/asm/inst.h 
b/arch/loongarch/include/asm/inst.h
index c00e151..fd31752 100644
--- a/arch/loongarch/include/asm/inst.h
+++ b/arch/loongarch/include/asm/inst.h
@@ -329,8 +329,8 @@ static inline bool is_pc_ins(union 
loongarch_instruction *ip)

  static inline bool is_branch_ins(union loongarch_instruction *ip)
  {
-       return ip->reg1i21_format.opcode >= beqz_op &&
-               ip->reg1i21_format.opcode <= bgeu_op;
+       return ((ip->word >> 26) & 0x3f) >= beqz_op &&
+               ((ip->word >> 26) & 0x3f) <= bgeu_op;
  }

Thanks,
Tiezhu
Jinyang He Dec. 17, 2022, 1:49 a.m. UTC | #3 
On 2022-12-16 14:11, Tiezhu Yang wrote:
>
>
> On 12/16/2022 11:18 AM, Jinyang He wrote:
>> Hi, Tiezhu,
>>
>>
>> On 2022-12-14 16:30, Tiezhu Yang wrote:
>>> The current definition of is_branch_ins() is not correct,
>>
>> But the branch instruction opcode only use the high 6 bits,
>
> Yes, I noticed that, the logic result of current code is right,
> but it seems a little strange (only consider reg1i21_format)
> at the first glance, the initial aim of this patch is to make
> it theoretically correct, maybe it is not the best change.
>
> I think we can neglect the instruction formats and check the
> high 6 bits instead, what do you think of the following change?

We defined many instruction format because of variable-width opcode 
field and parameter field. IMHO if there is no way to solve that problem 
really, keeping original codes is better. That depends on the 
maintainers, of course.


Thanks,

Jinyang


>
> diff --git a/arch/loongarch/include/asm/inst.h 
> b/arch/loongarch/include/asm/inst.h
> index c00e151..fd31752 100644
> --- a/arch/loongarch/include/asm/inst.h
> +++ b/arch/loongarch/include/asm/inst.h
> @@ -329,8 +329,8 @@ static inline bool is_pc_ins(union 
> loongarch_instruction *ip)
>
>  static inline bool is_branch_ins(union loongarch_instruction *ip)
>  {
> -       return ip->reg1i21_format.opcode >= beqz_op &&
> -               ip->reg1i21_format.opcode <= bgeu_op;
> +       return ((ip->word >> 26) & 0x3f) >= beqz_op &&
> +               ((ip->word >> 26) & 0x3f) <= bgeu_op;
>  }
>
> Thanks,
> Tiezhu
>
>
>
diff mbox series

Patch

diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
index c00e151..42be39be 100644
--- a/arch/loongarch/include/asm/inst.h
+++ b/arch/loongarch/include/asm/inst.h
@@ -329,8 +329,31 @@  static inline bool is_pc_ins(union loongarch_instruction *ip)
 
 static inline bool is_branch_ins(union loongarch_instruction *ip)
 {
-	return ip->reg1i21_format.opcode >= beqz_op &&
-		ip->reg1i21_format.opcode <= bgeu_op;
+	switch (ip->reg0i26_format.opcode) {
+	case b_op:
+	case bl_op:
+		return true;
+	}
+
+	switch (ip->reg1i21_format.opcode) {
+	case beqz_op:
+	case bnez_op:
+	case bceqz_op:
+		return true;
+	}
+
+	switch (ip->reg2i16_format.opcode) {
+	case jirl_op:
+	case beq_op:
+	case bne_op:
+	case blt_op:
+	case bge_op:
+	case bltu_op:
+	case bgeu_op:
+		return true;
+	}
+
+	return false;
 }
 
 static inline bool is_ra_save_ins(union loongarch_instruction *ip)