M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 10
Code Issues 98 Pull Requests 18 Releases Wiki Activity

core: Change RHS operand of bitshift operators to int32 #349

Merged
sb10q merged 2 commits from issue-336 into master 2023-11-09 12:16:21 +08:00
Conversation 29 Commits 2 Files Changed 6 +56 -4
2 changed files with 56 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 029dc6f61d - Show all commits

39
nac3core/src/codegen/expr.rs
View File

@ -298,8 +298,40 @@ impl<'ctx, 'a> CodeGenContext<'ctx, 'a> {
(Operator::BitOr, _) => self.builder.build_or(lhs, rhs, "or").into(), (Operator::BitOr, _) => self.builder.build_or(lhs, rhs, "or").into(),
(Operator::BitXor, _) => self.builder.build_xor(lhs, rhs, "xor").into(), (Operator::BitXor, _) => self.builder.build_xor(lhs, rhs, "xor").into(),
(Operator::BitAnd, _) => self.builder.build_and(lhs, rhs, "and").into(), (Operator::BitAnd, _) => self.builder.build_and(lhs, rhs, "and").into(),
(Operator::LShift, _) => self.builder.build_left_shift(lhs, rhs, "lshift").into(),
sb10q commented 2023-11-08 17:38:35 +08:00
Outdated
Review
Copy Link

It's become unclear here to which side of the operand signed refers to. I suggest adding a comment here to clarify what is going on.

It's become unclear here to which side of the operand ``signed`` refers to. I suggest adding a comment here to clarify what is going on.
(Operator::RShift, _) => self.builder.build_right_shift(lhs, rhs, true, "rshift").into(), // Sign-ness of bitshift operators are always determined by the left operand
(Operator::LShift, signed) | (Operator::RShift, signed) => {
sb10q commented 2023-11-08 17:32:03 +08:00
Outdated
Review
Copy Link

RHS is always 32, no?

RHS is always 32, no?
// RHS operand is always 32 bits
assert_eq!(rhs.get_type().get_bit_width(), 32);
sb10q commented 2023-11-02 14:00:24 +08:00
Outdated
Review
Copy Link

AFAICT this will break silently with negative shifts.

Python disallows them:

>>> 1 << (-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: negative shift count

So the RHS type should perhaps be uint32 below instead of int32, and we should check that the compiler reports a sensible error if the user enters a negative value there.

But I worry that uint32 would require explicit casts and a lot of typing, e.g. 1 << uint32(1) instead of just 1 << 1?

Alternatively we can simply support negative shifts since this is just a strict superset of Python as far as I can tell. But the zero-extend here should be replaced with a sign extend.

AFAICT this will break silently with negative shifts. Python disallows them: ``` >>> 1 << (-1) Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: negative shift count ``` So the RHS type should perhaps be uint32 below instead of int32, and we should check that the compiler reports a sensible error if the user enters a negative value there. But I worry that uint32 would require explicit casts and a lot of typing, e.g. ``1 << uint32(1)`` instead of just ``1 << 1``? Alternatively we can simply support negative shifts since this is just a strict superset of Python as far as I can tell. But the zero-extend here should be replaced with a sign extend.
let common_type = lhs.get_type();
let rhs = if common_type.get_bit_width() > 32 {
if signed {
self.builder.build_int_s_extend(rhs, common_type, "")
sb10q commented 2023-11-08 17:34:08 +08:00
Outdated
Review
Copy Link

Do we ever do this? Are there any int types smaller than 32-bit?

Do we ever do this? Are there any int types smaller than 32-bit?
} else {
self.builder.build_int_z_extend(rhs, common_type, "")
}
sb10q commented 2023-11-08 17:34:47 +08:00
Outdated
Review
Copy Link

Is LLVM able to optimize this away when the RHS is uint32?

Is LLVM able to optimize this away when the RHS is uint32?
sb10q commented 2023-11-08 17:35:05 +08:00
Outdated
Review
Copy Link

Or a constant?

Or a constant?
derppening commented 2023-11-09 11:50:34 +08:00
Outdated
Review
Copy Link

Yes, all test cases can be constant-folded using optimizations.

Yes, all test cases can be constant-folded using optimizations.
} else {
rhs
};
let rhs_gez = self.builder.build_int_compare(IntPredicate::SGE, rhs, common_type.const_zero(), "");
self.make_assert(
generator,
rhs_gez,
"ValueError",
"negative shift count",
[None, None, None],
self.current_loc
);
match *op {
Operator::LShift => self.builder.build_left_shift(lhs, rhs, "lshift").into(),
Operator::RShift => self.builder.build_right_shift(lhs, rhs, signed, "rshift").into(),
_ => unreachable!()
}
}
(Operator::FloorDiv, true) => self.builder.build_int_signed_div(lhs, rhs, "floordiv").into(), (Operator::FloorDiv, true) => self.builder.build_int_signed_div(lhs, rhs, "floordiv").into(),
(Operator::FloorDiv, false) => self.builder.build_int_unsigned_div(lhs, rhs, "floordiv").into(), (Operator::FloorDiv, false) => self.builder.build_int_unsigned_div(lhs, rhs, "floordiv").into(),
(Operator::Pow, s) => integer_power(generator, self, lhs, rhs, s).into(), (Operator::Pow, s) => integer_power(generator, self, lhs, rhs, s).into(),
@ -1085,6 +1117,9 @@ pub fn gen_binop_expr<'ctx, 'a, G: CodeGenerator>(
Ok(Some(ctx.gen_int_ops(generator, op, left_val, right_val, true).into())) Ok(Some(ctx.gen_int_ops(generator, op, left_val, right_val, true).into()))
} else if ty1 == ty2 && [ctx.primitives.uint32, ctx.primitives.uint64].contains(&ty1) { } else if ty1 == ty2 && [ctx.primitives.uint32, ctx.primitives.uint64].contains(&ty1) {
Ok(Some(ctx.gen_int_ops(generator, op, left_val, right_val, false).into())) Ok(Some(ctx.gen_int_ops(generator, op, left_val, right_val, false).into()))
} else if [Operator::LShift, Operator::RShift].contains(op) {
let signed = [ctx.primitives.int32, ctx.primitives.int64].contains(&ty1);
Ok(Some(ctx.gen_int_ops(generator, op, left_val, right_val, signed).into()))
} else if ty1 == ty2 && ctx.primitives.float == ty1 { } else if ty1 == ty2 && ctx.primitives.float == ty1 {
Ok(Some(ctx.gen_float_ops(op, left_val, right_val).into())) Ok(Some(ctx.gen_float_ops(op, left_val, right_val).into()))
} else if ty1 == ctx.primitives.float && ty2 == ctx.primitives.int32 { } else if ty1 == ctx.primitives.float && ty2 == ctx.primitives.int32 {

21
nac3core/src/typecheck/function_check.rs
View File

@ -2,7 +2,7 @@ use crate::typecheck::typedef::TypeEnum;
use super::type_inferencer::Inferencer; use super::type_inferencer::Inferencer;
use super::typedef::Type; use super::typedef::Type;
use nac3parser::ast::{self, Expr, ExprKind, Stmt, StmtKind, StrRef}; use nac3parser::ast::{self, Constant, Expr, ExprKind, Operator::{LShift, RShift}, Stmt, StmtKind, StrRef};
use std::{collections::HashSet, iter::once}; use std::{collections::HashSet, iter::once};
impl<'a> Inferencer<'a> { impl<'a> Inferencer<'a> {
@ -107,11 +107,28 @@ impl<'a> Inferencer<'a> {
self.check_expr(value, defined_identifiers)?; self.check_expr(value, defined_identifiers)?;
self.should_have_value(value)?; self.should_have_value(value)?;
} }
ExprKind::BinOp { left, right, .. } => { ExprKind::BinOp { left, op, right } => {
self.check_expr(left, defined_identifiers)?; self.check_expr(left, defined_identifiers)?;
self.check_expr(right, defined_identifiers)?; self.check_expr(right, defined_identifiers)?;
self.should_have_value(left)?; self.should_have_value(left)?;
self.should_have_value(right)?; self.should_have_value(right)?;
// Check whether a bitwise shift has a negative RHS constant value
if *op == LShift || *op == RShift {
if let ExprKind::Constant { value, .. } = &right.node {
let rhs_val = match value {
Constant::Int(v) => v,
_ => unreachable!(),
};
if *rhs_val < 0 {
return Err(format!(
"shift count is negative at {}",
right.location
));
}
}
}
} }
ExprKind::UnaryOp { operand, .. } => { ExprKind::UnaryOp { operand, .. } => {
self.check_expr(operand, defined_identifiers)?; self.check_expr(operand, defined_identifiers)?;