[v4,03/63] canonicalize compare instructions

Commit Message

Christopher Li March 24, 2017, 11:47 p.m. UTC

On Fri, Mar 24, 2017 at 1:11 AM, Luc Van Oostenryck
<[e-mail]> wrote:


>
> Yes, indeed.
> In fact, I think it need a name which explain even more
> the purpose. I'll see.

Sure, I am open to suggestion.

BTW, the previous function is call compare_opcode() what
it actually do is invert_compare(). I would just move the inverse
test outside of this function and call it invert_compare() or
invert_compare_opcode()

> Yes, it's kinda ugly.
> OTOH, given that jumps are cheaper and cheaper on modern CPU
> and memory accesses slower and slower, is what you propose
> really an optimization? And even if it would, does it matter?

Well, jump through a indirect table is quit different than jump
to a fix offset. I don't know what modern CPU does now days. It
used to be indirect jump will flush the instruction pipe line.



>
> My preference, if performance mattered here, would be to play
> with the bits of the opcode, something like:
>         #define OP_SET__EQUAL   (1 << ...)
>         #define OP_SET__SIGNED  (1 << ...)
>         #define OP_SET__LTHAN   ...
>         #define OP_SET__GTHAN   ...
>
>         ...
>
>         int swap_compare_opcode(opcode) { return opcode ^= (OP_SET__LTHAN|OP_SET__GTHAN); }

That is ugly. I don't want that.

I cook up an untested patch include here to show what I have in mind.
It should be very close to your case statement way of writing it.  You are
welcome to change it.

Chris

Comments

Luc Van Oostenryck March 25, 2017, 12:03 a.m. UTC | #1

On Sat, Mar 25, 2017 at 12:47 AM, Christopher Li <[e-mail]> wrote:
> On Fri, Mar 24, 2017 at 1:11 AM, Luc Van Oostenryck
> <[e-mail]> wrote:

>>
>> My preference, if performance mattered here, would be to play
>> with the bits of the opcode, something like:
>>         #define OP_SET__EQUAL   (1 << ...)
>>         #define OP_SET__SIGNED  (1 << ...)
>>         #define OP_SET__LTHAN   ...
>>         #define OP_SET__GTHAN   ...
>>
>>         ...
>>
>>         int swap_compare_opcode(opcode) { return opcode ^= (OP_SET__LTHAN|OP_SET__GTHAN); }
>
> That is ugly. I don't want that.
>
> I cook up an untested patch include here to show what I have in mind.
> It should be very close to your case statement way of writing it.  You are
> welcome to change it.

I already resent the serie, maybe 30 minutes ago or so.
but without this ugly thing you don't want.

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/simplify.c b/simplify.c
index 3f39819..6e113d9 100644
--- a/simplify.c
+++ b/simplify.c
@@ -438,22 +438,24 @@  static int compare_opcode(int opcode, int inverse)
 
 static int compare_swap(int opcode)
 {
-	switch (opcode) {
-	case OP_SET_EQ:	return OP_SET_EQ;
-	case OP_SET_NE:	return OP_SET_NE;
-
-	case OP_SET_LT:	return OP_SET_GT;
-	case OP_SET_LE:	return OP_SET_GE;
-	case OP_SET_GT:	return OP_SET_LT;
-	case OP_SET_GE:	return OP_SET_LE;
-
-	case OP_SET_A:	return OP_SET_B;
-	case OP_SET_AE:	return OP_SET_BE;
-	case OP_SET_B:	return OP_SET_A;
-	case OP_SET_BE:	return OP_SET_AE;
-	default:
-		return opcode;
-	}
+
+#define CMP_OFFSET(opcode) ((opcode) - OP_BINCMP)
+	static const int swap_opcodes[CMP_OFFSET(OP_BINCMP_END) + 1] = {
+		[CMP_OFFSET(OP_SET_EQ)] = OP_SET_EQ,
+		[CMP_OFFSET(OP_SET_NE)] = OP_SET_NE,
+
+		[CMP_OFFSET(OP_SET_LT)] = OP_SET_GT,
+		[CMP_OFFSET(OP_SET_GT)] = OP_SET_LT,
+		[CMP_OFFSET(OP_SET_LE)] = OP_SET_GE,
+		[CMP_OFFSET(OP_SET_GE)] = OP_SET_LE,
+
+		[CMP_OFFSET(OP_SET_A)] = OP_SET_B,
+		[CMP_OFFSET(OP_SET_B)] = OP_SET_A,
+		[CMP_OFFSET(OP_SET_AE)] = OP_SET_BE,
+		[CMP_OFFSET(OP_SET_BE)] = OP_SET_AE,
+	};
+	assert(opcode >= OP_BINCMP && opcode <= OP_BINCMP_END);
+	return swap_opcodes[CMP_OFFSET(opcode)];
 }
 
 static int simplify_seteq_setne(struct instruction *insn, long long value)