refactor: replace promote_operands with unified type promotion #237
refactor: replace promote_operands with unified type promotion #237omsherikar wants to merge 4 commits intoarxlang:mainfrom
promote_operands with unified type promotion #237Conversation
…ng#135) Drop fp_rank and promote_operands, use _unify_numeric_operands for both scalar and vector paths in BinaryOp.
There was a problem hiding this comment.
Pull request overview
This PR refactors numeric operand promotion in LLVMLiteIRVisitor by removing the legacy fp_rank/promote_operands path and introducing a unified _unify_numeric_operands helper that handles scalar↔vector shape unification and scalar type casting consistently in BinaryOp.
Changes:
- Removed
fp_rankandpromote_operands; added_unify_numeric_operandsplus supporting helpers for float-width selection and scalar/vector casting. - Updated
BinaryOpto unify numeric operands before vector/scalar op dispatch. - Updated and expanded tests to cover the new unification behavior (including scalar↔vector and float-rank cases).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/irx/builders/llvmliteir.py |
Introduces unified numeric operand unification/casting and wires it into BinaryOp; removes old promotion logic. |
tests/test_type_promotion.py |
Migrates existing promotion tests from promote_operands to _unify_numeric_operands. |
tests/test_llvmlite_helpers.py |
Adds targeted tests for scalar↔vector unification and float-rank widening; imports is_fp_type for assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if current_bits < target_bits: | ||
| return builder.fpext(value, target_ty, "fpext") | ||
| return builder.fptrunc(value, target_ty, "fptrunc") | ||
| return builder.sitofp(value, target_ty, "sitofp") |
|
|
||
| if current_width < target_width: |
|
@yuvi the pr is ready for review |
|
@omsherikar , CI seems to be failing |
Hii @yuvimittal it seems to be the env issue, its crashing I don't know why, how can I fix that? should I trigger the CI once again? |
|
@yuvimittal The CI are passing now, can you please review it? |
omsherikar
changed the title
promote_operands with unified type promotion
|
@omsherikar , i believe we had all the functionalities you added already in the codebase, could you explain more about what have you added |
I have just replaced two separate promotion paths with one unified method that handles scalars, vectors, and mixed scalar-vector and vector-vector cases. as the issue itself stated about unification |
|
@yuvimittal one more thing we can do is bring FMA into it too. but is it necessary? |
src/irx/builders/llvmliteir.py
Outdated
There was a problem hiding this comment.
Since _unify_numeric_operands already promotes operands to a common type, you no longer need the is_fp_type(llvm_lhs.type) or is_fp_type(llvm_rhs.type) guards , after unification, both sides already share the same type, so checking either one is sufficient. The or is now misleading.
There was a problem hiding this comment.
Should I just simplify those guards to check llvm_lhs.type only?
src/irx/builders/llvmliteir.py
Outdated
| return FLOAT16_BITS | ||
| if FP128Type is not None and isinstance(ty, FP128Type): | ||
| return FLOAT128_BITS | ||
| return getattr(ty, "width", 0) |
There was a problem hiding this comment.
returns 0 for unknown types, which would silently fall through to FLOAT_TYPE instead of raising. The old fp_rank `returned 0 and the caller would just proceed with mismatched types, so the behavior is similar, but it would be cleaner to raise explicitly on unknown FP types there.
There was a problem hiding this comment.
I will change it to raise explicitly on unknown FP types so we catch it.
| ) | ||
|
|
||
| # If both operands are LLVM vectors, handle as vector ops | ||
| if is_vector(llvm_lhs) and is_vector(llvm_rhs): |
There was a problem hiding this comment.
The vector block itself still has a duplicate size/element-type mismatch check that _unify_numeric_operands already enforces, those two raise Exception lines inside the vector block can be removed.
like wherever the unecessary exceptions are used, they should be removed, that is the point of unified right?
There was a problem hiding this comment.
my bad i missed that those were still there. I will remove those
Description
Dropped
fp_rankandpromote_operands, introduced_unify_numeric_operandsfor both scalar and vector paths in BinaryOp.solves #135