Skip to content

Comments

[Builder] Add F64Type support and SinOp for math operations#548

Merged
chhzh123 merged 2 commits intocornell-zhang:mainfrom
zzzDavid:f64-sinop
Feb 6, 2026
Merged

[Builder] Add F64Type support and SinOp for math operations#548
chhzh123 merged 2 commits intocornell-zhang:mainfrom
zzzDavid:f64-sinop

Conversation

@zzzDavid
Copy link
Contributor

@zzzDavid zzzDavid commented Feb 6, 2026

Summary

  • Add F64Type to the type check for scalar math operations in builder.py, so float64 arguments are recognized alongside float32 and integer types
  • Add math.SinOp to the supported math operation map, enabling allo.sin() for all float types
  • Add dedicated test cases for sin (float32 and float64) and float64 math/trig operations

Split out from #287 per review feedback.

Test plan

  • test_sin_float32 — verifies allo.sin() works with float32 arrays
  • test_sin_float64 — verifies allo.sin() works with float64 arrays
  • test_float64_math_ops — verifies allo.exp(), allo.log(), allo.sqrt() with float64
  • test_float64_trig_ops — verifies allo.sin(), allo.cos(), allo.tan() with float64

🤖 Generated with Claude Code

Add F64Type to the type check for scalar math operations so that
float64 arguments are recognized alongside float32 and integer types.
Also add math.SinOp to the supported operation map. These changes
enable math operations (exp, log, sqrt, sin, cos, tan, etc.) to work
with float64 types, and add sin() support for all float types.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 6, 2026 19:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends Allo’s IR builder to recognize float64 in scalar math op lowering and adds sin support via the MLIR math dialect, with new tests covering sin and other math/trig operations on float64.

Changes:

  • Add F64Type to the scalar math-op type check in allo/ir/builder.py.
  • Add "sin": math_d.SinOp to the scalar math operation map.
  • Add tests for allo.sin() on float32/float64, plus additional float64 math/trig coverage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
allo/ir/builder.py Expands scalar math-op lowering to accept f64 and adds sin lowering.
tests/test_types.py Adds new regression tests for sin and other math/trig ops on float64.
Comments suppressed due to low confidence (1)

allo/ir/builder.py:3126

  • opcls can be None when fn_name isn’t in the mapping, but this branch still unconditionally calls opcls(...), which will raise a TypeError instead of the intended unsupported-function error. Add an explicit check for opcls is None and fall through to the later handling / raise a clear RuntimeError for unsupported scalar math ops.
                opcls = {
                    "exp": math_d.ExpOp,
                    "log": math_d.LogOp,
                    "log2": math_d.Log2Op,
                    "log10": math_d.Log10Op,
                    "sqrt": math_d.SqrtOp,
                    "sin": math_d.SinOp,
                    "cos": math_d.CosOp,
                    "tan": math_d.TanOp,
                    "tanh": math_d.TanhOp,
                    "power": math_d.PowFOp,
                    "abs": math_d.AbsIOp,
                }.get(fn_name)
                return opcls(
                    *[ASTTransformer.get_mlir_op_result(ctx, x) for x in new_args],
                    ip=ctx.get_ip(),
                )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add dtype assertions to float64 tests to ensure the pipeline does not
silently downcast to float32.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@zzzDavid zzzDavid mentioned this pull request Feb 6, 2026
4 tasks
Copy link
Member

@chhzh123 chhzh123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@chhzh123 chhzh123 merged commit 6b44001 into cornell-zhang:main Feb 6, 2026
1 check passed
@zzzDavid zzzDavid deleted the f64-sinop branch February 6, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants