[PATCH] sparse, llvm: Simplify comparison op code generation

[PATCH] sparse, llvm: Simplify comparison op code generation

[Pekka Enberg]

Linus writes:

  On Tue, Nov 22, 2011 at 9:40 AM, Pekka Enberg <penberg@kernel.org> wrote:
  > This patch implements LLVM code generation for OP_SET_LE, OP_SET_GE, OP_SET_BE,
  > and OP_SET_AE.

  Ugh.

  Can't you just do it with a single statement like

     target = LLVMBuildICmp(fn->builder, translate_op(op), lhs, rhs,
target_name);

  instead of having that case-statement where every case does the same thing?

  The translate_op() thing should be trivial too, just something like

    static int translate_op(int sparse_op)
    {
        static const int trans_tbl[] = {
            .[OP_SET_LE] = LLVMIntSLE,
        ...
         };
         return trans_tbl[sparse_op];
    }

  or whatever. No?

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 sparse-llvm.c |   56 +++++++++++++++++++++++---------------------------------
 1 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 700a7a4..4ef02a1 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -391,6 +391,24 @@ static LLVMTypeRef pseudo_type(struct function *fn, struct instruction *insn, ps
 	return result;
 }
 
+static LLVMIntPredicate translate_op(int opcode)
+{
+	static const LLVMIntPredicate trans_tbl[] = {
+		[OP_SET_EQ]	= LLVMIntEQ,
+		[OP_SET_NE]	= LLVMIntNE,
+		[OP_SET_LE]	= LLVMIntSLE,
+		[OP_SET_GE]	= LLVMIntSGE,
+		[OP_SET_LT]	= LLVMIntSLT,
+		[OP_SET_GT]	= LLVMIntSGT,
+		[OP_SET_B]	= LLVMIntULT,
+		[OP_SET_A]	= LLVMIntUGT,
+		[OP_SET_BE]	= LLVMIntULE,
+		[OP_SET_AE]	= LLVMIntUGE,
+	};
+
+	return trans_tbl[opcode];
+}
+
 static void output_op_binary(struct function *fn, struct instruction *insn)
 {
 	LLVMValueRef lhs, rhs, target;
@@ -493,40 +511,12 @@ static void output_op_binary(struct function *fn, struct instruction *insn)
 	}
 
 	/* Binary comparison */
-	case OP_SET_EQ:
-		assert(!symbol_is_fp_type(insn->type));
-		target = LLVMBuildICmp(fn->builder, LLVMIntEQ, lhs, rhs, target_name);
-		break;
-	case OP_SET_NE:
-		assert(!symbol_is_fp_type(insn->type));
-		target = LLVMBuildICmp(fn->builder, LLVMIntNE, lhs, rhs, target_name);
-		break;
-	case OP_SET_LE:
-		target = LLVMBuildICmp(fn->builder, LLVMIntSLE, lhs, rhs, target_name);
-		break;
-	case OP_SET_GE:
-		target = LLVMBuildICmp(fn->builder, LLVMIntSGE, lhs, rhs, target_name);
-		break;
-	case OP_SET_LT:
-		assert(!symbol_is_fp_type(insn->type));
-		target = LLVMBuildICmp(fn->builder, LLVMIntSLT, lhs, rhs, target_name);
-		break;
-	case OP_SET_GT:
-		assert(!symbol_is_fp_type(insn->type));
-		target = LLVMBuildICmp(fn->builder, LLVMIntSGT, lhs, rhs, target_name);
-		break;
-	case OP_SET_B:
-		target = LLVMBuildICmp(fn->builder, LLVMIntULT, lhs, rhs, target_name);
-		break;
-	case OP_SET_A:
-		target = LLVMBuildICmp(fn->builder, LLVMIntUGT, lhs, rhs, target_name);
-		break;
-	case OP_SET_BE:
-		target = LLVMBuildICmp(fn->builder, LLVMIntULE, lhs, rhs, target_name);
-		break;
-	case OP_SET_AE:
-		target = LLVMBuildICmp(fn->builder, LLVMIntUGE, lhs, rhs, target_name);
+	case OP_BINCMP ... OP_BINCMP_END: {
+		LLVMIntPredicate op = translate_op(insn->opcode);
+
+		target = LLVMBuildICmp(fn->builder, op, lhs, rhs, target_name);
 		break;
+	}
 	default:
 		assert(0);
 		break;
-- 
1.7.6.4

Re: [PATCH] sparse, llvm: Simplify comparison op code generation

[Linus Torvalds]

On Tue, Nov 22, 2011 at 9:58 AM, Pekka Enberg <penberg@kernel.org> wrote:
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>

Hey, I obviously like it since I suggested it, but I wonder if you
shouldn't handle the FP cases too?

You have lost the

  assert(!symbol_is_fp_type(insn->type));

but I do suspect that the same code should be able to largely just
handle the FP cases (you'd obviously need to translate the sparse
OP_SET_NE things into the proper LLVM LLVMfpEQ or whatever LLVM does).

That said, if you don't handle the FP cases, I think you should
re-introduce the assert().

                      Linus

Re: [PATCH] sparse, llvm: Simplify comparison op code generation

[Pekka Enberg]

> On Tue, Nov 22, 2011 at 9:58 AM, Pekka Enberg <penberg@kernel.org> wrote:
>>
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>

On Tue, Nov 22, 2011 at 8:05 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Hey, I obviously like it since I suggested it, but I wonder if you
> shouldn't handle the FP cases too?
>
> You have lost the
>
>  assert(!symbol_is_fp_type(insn->type));
>
> but I do suspect that the same code should be able to largely just
> handle the FP cases (you'd obviously need to translate the sparse
> OP_SET_NE things into the proper LLVM LLVMfpEQ or whatever LLVM does).
>
> That said, if you don't handle the FP cases, I think you should
> re-introduce the assert().

Actually, the assert() doesn't catch anything. With something like this:

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 4ef02a1..3806876 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -514,6 +514,8 @@ static void output_op_binary(struct function *fn,
struct instruction *insn)
        case OP_BINCMP ... OP_BINCMP_END: {
                LLVMIntPredicate op = translate_op(insn->opcode);

+               assert(!symbol_is_fp_type(insn->type));
+
                target = LLVMBuildICmp(fn->builder, op, lhs, rhs, target_name);
                break;
        }
diff --git a/validation/backend/cmp-ops.c b/validation/backend/cmp-ops.c
index 7bbc81c..d8b5e9a 100644
--- a/validation/backend/cmp-ops.c
+++ b/validation/backend/cmp-ops.c
@@ -8,6 +8,11 @@ static int setne(int x, int y)
        return x != y;
 }

+static int fsetl(float x, float y)
+{
+       return x < y;
+}
+
 static int setl(int x, int y)
 {
        return x < y;

You hit this assertion in LLVM:

[penberg@tux sparse]$ ./sparse-llvm validation/backend/cmp-ops.c
sparse-llvm: /home/penberg/llvm/include/llvm/Instructions.h:949:
llvm::ICmpInst::ICmpInst(llvm::CmpInst::Predicate, llvm::Value*,
llvm::Value*, const llvm::Twine&): Assertion
`(getOperand(0)->getType()->isIntOrIntVectorTy() ||
getOperand(0)->getType()->isPointerTy()) && "Invalid operand types for
ICmp instruction"' failed.
Aborted

I think that might be a generic sparse floating point issue but I
didn't look too closely.

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

Re: [PATCH] sparse, llvm: Simplify comparison op code generation

[Linus Torvalds]

On Tue, Nov 22, 2011 at 10:18 AM, Pekka Enberg <penberg@kernel.org> wrote:
>
> I think that might be a generic sparse floating point issue but I
> didn't look too closely.

No, it looks like sparse is doing fine, but because you don't generate
the FP versions of the comparison ops, LLVM is unhappy with getting an
integer comparison for a floating point value.

Enhancing the translate_op() function to generate the proper FP
comparisons for LLVM would probably make it "JustWork(tm)".

But I don't really claim to know LLVM.

                    Linus

Re: [PATCH] sparse, llvm: Simplify comparison op code generation

[Pekka Enberg]

On Tue, Nov 22, 2011 at 10:18 AM, Pekka Enberg <penberg@kernel.org> wrote:
>> I think that might be a generic sparse floating point issue but I
>> didn't look too closely.

On Tue, Nov 22, 2011 at 8:26 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> No, it looks like sparse is doing fine, but because you don't generate
> the FP versions of the comparison ops, LLVM is unhappy with getting an
> integer comparison for a floating point value.
>
> Enhancing the translate_op() function to generate the proper FP
> comparisons for LLVM would probably make it "JustWork(tm)".

Unfortunately LLVM API requires LLVMBuildFCmp() for floats so it's not
as simple as that. I sent an incremental patch as a separate email
that adds FP support for comparison ops.

Re: [PATCH] sparse, llvm: Simplify comparison op code generation

[Linus Torvalds]

On Tue, Nov 22, 2011 at 11:57 AM, Pekka Enberg <penberg@kernel.org> wrote:
>
> Unfortunately LLVM API requires LLVMBuildFCmp() for floats so it's not
> as simple as that. I sent an incremental patch as a separate email
> that adds FP support for comparison ops.

Yeah, saw that, and it looked sane to me.

                Linus