feat: unify numeric operand promotion

[omsherikar]

Pull Request description

Centralize numeric operand promotion for binops .
Adds _unify_numeric_operands plus helper tests so ints/floats/vectors all go through one path before LLVM emission, preventing mismatched widths or scalar/vector handling bugs.

Solve #135

How to test these changes

• python -m pytest tests/test_llvmlite_helpers.py -v
• pre-commit run --files src/irx/builders/llvmliteir.py tests/test_llvmlite_helpers.py

Pull Request checklists

This PR is a:

• bug-fix
• new feature
• maintenance

About this PR:

• it includes tests.
• the tests are executed on CI.
• the tests generate log file(s) (path).
• pre-commit hooks were executed locally.
• this PR requires a project documentation update.

Author's checklist:

• I have reviewed the changes and it contains no misspelling.
• The code is well commented, especially in the parts that contain more complexity.
• New and old tests passed locally.

Additional information

N/A

Reviewer's checklist

[github-actions]

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

src/irx/builders/llvmliteir.py

• Correctness: signedness is ignored in casts.
  • _cast_value_to_type() always uses sitofp and sext, which is wrong for unsigned integers (will corrupt values). You need sign-awareness from the AST/type system. Suggest adding a signed: bool flag (default True) and using uitofp/zext when unsigned. (L.487)
    def _cast_value_to_type(self, value: ir.Value, target_scalar_ty: ir.Type, signed: bool = True) -> ir.Value:
    """Sign-aware cast of scalars or vectors to the target scalar type."""
    # use builder.uitofp(...) if not signed
    # use builder.zext(...) if not signed
• Correctness: FP128/X86_FP80 handling is unsafe/incomplete.
  • FP128Type is referenced directly and may be undefined -> NameError at runtime. Use hasattr(ir, "FP128Type") and refer to ir.FP128Type instead. (L.460, L.476)
    def _float_type_from_width(self, width: int) -> ir.Type:
    """Create float type from bit width (safe for optional types)."""
    # use hasattr(ir, "FP128Type") and ir.FP128Type()
  • _float_type_from_width() silently falls back to 32-bit for widths in (64,128), e.g., x86_fp80, causing precision loss. Either support ir.X86_FP80Type explicitly or at least fall back to DOUBLE_TYPE instead of FLOAT_TYPE for >64 and <128. Also detect X86_FP80 in _float_bit_width(). (L.460, L.476)
    def _float_bit_width(self, ty: ir.Type) -> int:
    """Bit width including platform extended types."""
    # handle ir.X86_FP80Type if available
• Behavior change/perf: scalar-vector promotion now widens the vector element type to the widest float (e.g., float op double -> double). This can break downstream typing assumptions and degrade perf on targets without vector f64. Consider preserving the vector element type when one operand is a vector (only cast/splat the scalar), unless your type system specifies “widest wins.” (L.441)
  def _unify_numeric_operands(self, lhs: ir.Value, rhs: ir.Value) -> tuple[ir.Value, ir.Value]:
  """Prefer vector element type when only one operand is vector."""
  # if exactly one is vector: target_scalar_ty = that vector's element type

LGTM!

---

tests/test_llvmlite_helpers.py

• Potential false positive: test_unify_int_and_float_scalars_returns_float allows widened_int to be any FP type, which can mask a bug where operands are not actually unified to the same type. Tighten the assertions to ensure both operands are the same type and that it matches FLOAT_TYPE as the docstring states. (L.153-L.155)
  Replace:
  assert is_fp_type(widened_int.type)
  assert widened_float.type == visitor._llvm.FLOAT_TYPE
  With:
  assert widened_int.type == visitor._llvm.FLOAT_TYPE
  assert widened_float.type == visitor._llvm.FLOAT_TYPE
  assert widened_int.type == widened_float.type

• Maintainability risk: tests hinge on private APIs (visitor._unify_numeric_operands and visitor._llvm.*). Consider exposing a small public helper (or alias) to stabilize the contract and avoid brittle coupling to internals. E.g., add a public wrapper in LLVMLiteIRVisitor and update these calls. (L.106, L.130, L.148)

---

[omsherikar]

@xmnlab @yuvimittal please have a look

[github-actions]

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

src/irx/builders/llvmliteir.py

• Correctness: The numeric unification runs before operator classification, so bitwise/shift ops with mixed int/float will silently cast ints to float (sitofp) and later fail or miscompile. Gate unification to arithmetic-only ops. (L.670)
  Suggested change:
  def _should_unify_as_arith(self, node: astx.BinaryOp) -> bool:
  """Decide if numeric unification applies (arith only, not bitwise/shifts)."""
  # implement based on node.op
  return node.op in {astx.Op.ADD, astx.Op.SUB, astx.Op.MUL, astx.Op.DIV, astx.Op.MOD, astx.Op.POW}

  in visit(BinaryOp):

  if self._should_unify_as_arith(node) and self._is_numeric_value(llvm_lhs) and self._is_numeric_value(llvm_rhs):
  llvm_lhs, llvm_rhs = self._unify_numeric_operands(llvm_lhs, llvm_rhs)

• Correctness: Signedness is ignored during integer widening and int->float casts (sext/sitofp). This will break unsigned semantics (e.g., OR/AND after a sext, or uitofp required). Thread signedness from AST/type info and choose zext/uitofp when appropriate. (L.533, L.557)
  Suggested change:
  def _cast_value_to_type(self, value: ir.Value, target_scalar_ty: ir.Type, *, signed: bool) -> ir.Value:
  """Cast scalars or vectors to the target scalar type with signedness awareness."""
  builder = self._llvm.ir_builder
  value_is_vec = is_vector(value)
  lanes = value.type.count if value_is_vec else None
  current_scalar_ty = value.type.element if value_is_vec else value.type
  target_ty = ir.VectorType(target_scalar_ty, lanes) if value_is_vec else target_scalar_ty

  if current_scalar_ty == target_scalar_ty and value.type == target_ty:
      return value
  
  current_is_float = is_fp_type(current_scalar_ty)
  target_is_float = is_fp_type(target_scalar_ty)
  
  if target_is_float:
      if current_is_float:
          current_bits = self._float_bit_width(current_scalar_ty)
          target_bits = self._float_bit_width(target_scalar_ty)
          if current_bits == target_bits:
              return builder.bitcast(value, target_ty) if value.type != target_ty else value
          return builder.fpext(value, target_ty, "fpext") if current_bits < target_bits else builder.fptrunc(value, target_ty, "fptrunc")
      return (builder.sitofp if signed else builder.uitofp)(value, target_ty, "itofp")
  
  if current_is_float:
      raise Exception("Cannot implicitly convert floating-point to integer")
  
  current_width = getattr(current_scalar_ty, "width", 0)
  target_width = getattr(target_scalar_ty, "width", 0)
  if current_width == target_width:
      return builder.bitcast(value, target_ty) if value.type != target_ty else value
  return (builder.sext if signed else builder.zext)(value, target_ty, "ext") if current_width < target_width else builder.trunc(value, target_ty, "trunc")
  

  • Pass signed=... from AST type info where calling _cast_value_to_type/_unify_numeric_operands.

• Correctness: i1 booleans are treated as numeric here and may be promoted/splat, which can corrupt logical ops. Exclude i1 from _is_numeric_value. (L.435)
  Suggested change:
  def _is_numeric_value(self, value: ir.Value) -> bool:
  """Return True if value represents an int/float scalar or vector (excluding i1)."""
  if is_vector(value):
  elem_ty = value.type.element
  if isinstance(elem_ty, ir.IntType) and getattr(elem_ty, "width", 0) == 1:
  return False
  return isinstance(elem_ty, ir.IntType) or is_fp_type(elem_ty)
  base_ty = value.type
  if isinstance(base_ty, ir.IntType) and getattr(base_ty, "width", 0) == 1:
  return False
  return isinstance(base_ty, ir.IntType) or is_fp_type(base_ty)

• Portability: FP128Type() may not be legal on all targets even if the class exists. Prefer a target-aware handle if available (e.g., self._llvm.FP128_TYPE) and only select if the module/target supports it; otherwise fall back to DOUBLE. (L.470)
  Suggested change:
  def _float_type_from_width(self, width: int) -> ir.Type:
  """Select a usable float type for the current target."""
  if width <= FLOAT16_BITS and hasattr(self._llvm, "FLOAT16_TYPE"):
  return self._llvm.FLOAT16_TYPE
  if width <= FLOAT32_BITS:
  return self._llvm.FLOAT_TYPE
  if width <= FLOAT64_BITS:
  return self._llvm.DOUBLE_TYPE
  if hasattr(self._llvm, "FP128_TYPE"):
  return self._llvm.FP128_TYPE
  return self._llvm.DOUBLE_TYPE

---

tests/test_llvmlite_helpers.py

LGTM!

---

[github-actions]

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

src/irx/builders/llvmliteir.py

• Possible NameError on FP128Type: you reference FP128Type directly without importing it. Use ir.FP128Type to avoid runtime errors. Update both checks and constructors. (L.472, L.486)

  • Replace:
    • if FP128Type is not None and width >= FLOAT128_BITS:
    • if FP128Type is not None and isinstance(ty, FP128Type):
  • With:
    • if hasattr(ir, "FP128Type") and width >= FLOAT128_BITS:
    • if hasattr(ir, "FP128Type") and isinstance(ty, ir.FP128Type):

• Incorrect downcast of non-64/128 FP types: _float_type_from_width falls back to 32-bit float for widths >64 and <128 (e.g., x86 80-bit). Add explicit support for X86_FP80 to prevent precision loss. (L.468)

  • Suggested change:
    def _float_type_from_width(self, width: int) -> ir.Type:
    """Select float type by bit-width"""
    if width <= FLOAT16_BITS and hasattr(self._llvm, "FLOAT16_TYPE"):
    return self._llvm.FLOAT16_TYPE
    if width <= FLOAT32_BITS:
    return self._llvm.FLOAT_TYPE
    if width <= FLOAT64_BITS:
    return self._llvm.DOUBLE_TYPE
    if hasattr(ir, "X86_FP80Type") and width <= 80:
    return ir.X86_FP80Type()
    if hasattr(ir, "FP128Type") and width >= FLOAT128_BITS:
    return ir.FP128Type()
    return self._llvm.FLOAT_TYPE

• Signedness bugs in integer promotions/casts:

  • Widening ints uses sext, which is wrong for unsigned/boolean operands; and int->float uses sitofp, which is wrong for unsigned. At minimum, treat i1 as unsigned to avoid -1 for True. (L.520, L.536)
  • Suggested changes:
    def _cast_value_to_type(self, value: ir.Value, target_scalar_ty: ir.Type) -> ir.Value:
    """Cast scalars or vectors to the target scalar type"""
    ...
    if target_is_float:
    if current_is_float:
    ...
    # int -> float
    if isinstance(current_scalar_ty, ir.IntType) and getattr(current_scalar_ty, "width", 0) == 1:
    return builder.uitofp(value, target_ty, "uitofp") # bools are unsigned
    return builder.sitofp(value, target_ty, "sitofp")
    ...
    # int -> wider int
    if current_width < target_width:
    if current_width == 1:
    return builder.zext(value, target_ty, "zext") # preserve boolean semantics
    return builder.sext(value, target_ty, "sext")

• Vector element width selection: when neither operand is float, you pick max(lhs_width, rhs_width, 1). If either width lookup fails (returns 0), this can produce i1 and silently narrow. Consider asserting widths > 0 for ints to avoid accidental i1. (L.449)

  • Suggested guard:
    def _unify_numeric_operands(self, lhs: ir.Value, rhs: ir.Value) -> tuple[ir.Value, ir.Value]:
    """Ensure numeric operands share shape and scalar type"""
    ...
    if not (isinstance(lhs_base_ty, ir.IntType) and isinstance(rhs_base_ty, ir.IntType)):
    ...
    lhs_width = getattr(lhs_base_ty, "width", 0)
    rhs_width = getattr(rhs_base_ty, "width", 0)
    if lhs_width <= 0 or rhs_width <= 0:
    raise Exception("Unsupported integer type without width")
    target_scalar_ty = ir.IntType(max(lhs_width, rhs_width))

---

tests/test_llvmlite_helpers.py

LGTM!

---