fix: make fast-math toggles work

[omsherikar]

Pull Request description

This PR fixes issue #136 by implementing the missing set_fast_math method and adding support for applying fast-math flags to floating-point instructions. Previously, calls to self.set_fast_math(True/False) in visit(BinaryOp) would crash because the method did not exist in LLVMLiteIRVisitor.

The fix includes:

• Implementation of set_fast_math(enabled: bool) to track fast-math state
• New helper _apply_fast_math(inst) to conditionally attach the "fast" flag to floating-point instructions
• Integration of fast-math flag application into all floating-point operations (fadd, fsub, fmul, fdiv, fma)
• Unit tests to verify fast-math flags are correctly applied when enabled

Solve #136

How to test these changes

• Run makim tests.unit to execute the unit test suite
• The new test test_set_fast_math_marks_float_ops verifies that fast-math flags are applied to floating-point operations when set_fast_math(True) is called
• Verify that fast-math flags are not applied when set_fast_math(False) is called

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

The implementation tracks fast-math state via self._fast_math_enabled and applies flags only to floating-point instructions (both scalar and vector). The _apply_fast_math helper checks if the instruction type is a floating-point type before attaching the "fast" flag, ensuring that integer operations are not affected.

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved .

[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

• Risk: Applying fast-math via inst.flags on call instructions (e.g., llvm.fma) may be unsupported or generate invalid IR; many llvmlite call instructions don’t expose flags and will raise AttributeError at runtime. Make _apply_fast_math robust and only set flags when supported. Suggested change (L.441):

  def _apply_fast_math(self, inst: ir.Instruction) -> None:
  """Attach fast-math flags when enabled and applicable."""
  if not self._fast_math_enabled:
  return
  ty = inst.type
  if isinstance(ty, ir.VectorType):
  if not is_fp_type(ty.element):
  return
  elif not is_fp_type(ty):
  return
  # Prefer instruction-level fastmath API if present
  if hasattr(inst, "fastmath"):
  setattr(inst, "fastmath", "fast")
  return
  flags = getattr(inst, "flags", None)
  if isinstance(flags, list) and "fast" not in flags:
  flags.append("fast")

• Maintainability/coverage: You’re manually touching many FP ops; consider also toggling the builder’s global fastmath to avoid misses and ensure future FP ops inherit it. Suggested change (L.429):

  def set_fast_math(self, enabled: bool) -> None:
  """Enable/disable fast-math flags for subsequent FP instructions."""
  self._fast_math_enabled = enabled
  # Also set builder-level fastmath if available to cover future ops
  fm = "fast" if enabled else None
  ib = getattr(getattr(self, "_llvm", None), "ir_builder", None)
  if ib is not None and hasattr(ib, "fastmath"):
  setattr(ib, "fastmath", fm)

---

tests/test_llvmlite_helpers.py

• The assertions rely on "fast" being present in inst.flags, which may not hold across llvmlite versions (some expose fast-math via inst.fastmath or expand the composite into granular flags like nnan/ninf/nsz/arcp/contract/afn/reassoc). Make the check version-tolerant and accept either the composite or granular flags.
  Suggested change (L.102-L.105):
  def _get_fp_flags(inst: object) -> tuple[str, ...]:
  """Return fast-math or flags in a version-tolerant way."""
  fm = getattr(inst, "fastmath", None)
  if fm:
  return tuple(fm)
  return tuple(getattr(inst, "flags", ()))

• Update assertions to use the helper (L.110, L.115):
  flags_fast = _get_fp_flags(inst_fast)
  assert any(f in flags_fast for f in ("fast", "nnan", "ninf", "nsz", "arcp", "contract", "afn", "reassoc"))

  flags_normal = _get_fp_flags(inst_normal)
  assert all(f not in flags_normal for f in ("fast", "nnan", "ninf", "nsz", "arcp", "contract", "afn", "reassoc"))

• This test reaches into private internals (visitor._llvm.ir_builder, visitor._apply_fast_math). That’s brittle and couples tests to implementation details. If possible, prefer asserting via a public API that emits the instruction with the expected flags, or add a small public helper to apply/check fast-math on an instruction.

---

[omsherikar]

@xmnlab @yuvimittal Please review this

[xmnlab]

hi @omsherikar , thanks for working on that.
I will review it this week. thanks for your patience.

[omsherikar]

hi @omsherikar , thanks for working on that. I will review it this week. thanks for your patience.

Sure @xmnlab, Thank you!

[omsherikar]

@xmnlab Please have a look into it

[omsherikar]

@yuvimittal Have a look at it

[yuvimittal]

inst.flags might be None, immutable or absent, so it can cause attributeError or TypeError.

[omsherikar]

Thanks for the feedback @yuvimittal. and sorry for the delay .Should I handle it with a try-except block?

[omsherikar]

I've added defensive handling using getattr and try/except so it won't crash if flags is None or immutable.

[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

• High-risk bug: using inst.flags to attach "fast" is incorrect for FP ops in llvmlite and may either be ignored or break IR verification. Floating-point fast-math must use FastMathFlags via the inst.fastmath attribute. Replace the tail of _apply_fast_math accordingly. (L.439)

  def _apply_fast_math(self, inst: ir.Instruction) -> None:
  """Attach fast-math flags when enabled and applicable."""
  if not self._fast_math_enabled:
  return
  ty = inst.type
  if isinstance(ty, ir.VectorType):
  if not is_fp_type(ty.element):
  return
  elif not is_fp_type(ty):
  return
  fm = ir.FastMathFlags()
  fm.set_fast()
  inst.fastmath = fm

• Optional simplification with broader coverage: set/clear builder-level fastmath once in set_fast_math to automatically apply to subsequent FP instructions (including calls to intrinsics), reducing the need to remember _apply_fast_math at each site. (L.428)

  def set_fast_math(self, enabled: bool) -> None:
  """Enable/disable fast-math flags for subsequent FP instructions."""
  self._fast_math_enabled = enabled
  if enabled:
  fm = ir.FastMathFlags()
  fm.set_fast()
  self._llvm.ir_builder.fastmath = fm
  else:
  self._llvm.ir_builder.fastmath = None

---

tests/test_llvmlite_helpers.py

• Tests rely on private internals (_llvm, FLOAT_TYPE, ir_builder, _apply_fast_math), which makes them brittle to refactors. At minimum, avoid the private type constant. Use llvmlite’s public type instead (L.104):
  float_ty: ir.types.FloatType = ir.FloatType()

• Asserting on inst.flags is likely to be fragile across llvmlite versions. Assert on the emitted IR instead to reduce coupling (L.111, L.117):
  assert "fast" in str(inst_fast)
  assert "fast" not in str(inst_normal)

---

[Copilot]

Pull request overview

This PR fixes issue #136 by implementing fast-math flag support for floating-point operations in the LLVM IR builder. The implementation adds a set_fast_math method to track fast-math state and a helper to conditionally apply the "fast" flag to floating-point instructions.

Key Changes:

• Added set_fast_math(enabled: bool) method and _fast_math_enabled instance variable to track fast-math state
• Implemented _apply_fast_math(inst) helper that checks instruction types and applies flags appropriately
• Integrated fast-math flag application into all floating-point operations (fadd, fsub, fmul, fdiv, fma)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/irx/builders/llvmliteir.py	Adds fast-math state tracking, helper method, and integrates flag application into FP operations
tests/test_llvmlite_helpers.py	Adds unit test verifying fast-math flags are correctly applied when enabled and absent when disabled

---

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

[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

• High risk: _apply_fast_math mutates inst.flags with "fast". In llvmlite, FP fast-math is controlled via the instruction’s fastmath attribute, not generic flags; appending "fast" to flags may be ignored by FP ops and calls, so fast-math likely never takes effect. Use inst.fastmath = "fast" (and only fall back to flags if you must for legacy). Suggested replacement (L.446):

  def _apply_fast_math(self, inst: ir.Instruction) -> None:
  """Attach fast-math flags when enabled and applicable."""
  if not self._fast_math_enabled:
  return
  ty = inst.type
  if isinstance(ty, ir.VectorType):
  if not is_fp_type(ty.element):
  return
  elif not is_fp_type(ty):
  return
  if hasattr(inst, "fastmath"):
  inst.fastmath = "fast"
  else:
  flags = getattr(inst, "flags", None)
  if isinstance(flags, list) and "fast" not in flags:
  flags.append("fast")

---

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

Bug: fast-math flags are applied to inst.flags, but in llvmlite FP ops/calls use the fastmath attribute, not flags. As written, this likely no-ops for fadd/fsub/fmul/fdiv/call. Please set inst.fastmath (and assign, not just mutate) and handle vector element attr robustly. Suggested fix:

(L.435)
def _apply_fast_math(self, inst: ir.Instruction) -> None:
"""Attach fast-math flags when enabled and applicable."""
if not self._fast_math_enabled:
return
ty = inst.type
if isinstance(ty, ir.VectorType):
elem = getattr(ty, "element", getattr(ty, "element_type", None))
if elem is None or not is_fp_type(elem):
return
elif not is_fp_type(ty):
return
fm = getattr(inst, "fastmath", None)
if fm is None:
return
if isinstance(fm, (list, tuple)):
if "fast" not in fm:
inst.fastmath = list(fm) + ["fast"]
else:
inst.fastmath = ["fast"]

---

tests/test_llvmlite_helpers.py

• The test is brittle by asserting the umbrella flag string "fast". If the implementation switches to specific FMF subsets (nnan/ninf/nsz/arcp/contract/reassoc), this will fail despite correct behavior. Suggest relaxing to “any FMF present” and guarding for llvmlite variants that expose flags differently. (L.110, L.115)
  Replace:
  assert "fast" in inst_fast.flags
  assert "fast" not in inst_normal.flags
  With:
  fmf: set[str] = {"fast", "nnan", "ninf", "nsz", "arcp", "contract", "reassoc"} # type: ignore[var-annotated]
  assert bool(fmf & set(getattr(inst_fast, "flags", getattr(inst_fast, "fastmath", []))))
  assert not bool(fmf & set(getattr(inst_normal, "flags", getattr(inst_normal, "fastmath", []))))

• The test couples to private internals (visitor._llvm.FLOAT_TYPE, visitor._llvm.ir_builder, visitor._apply_fast_math). This increases fragility and can break with internal refactors. If possible, prefer public APIs, e.g., use ir.FloatType() over the private constant. (L.103)
  Replace:
  float_ty = visitor._llvm.FLOAT_TYPE
  With:
  float_ty: ir.Type = ir.FloatType() # """Use public llvmlite type to avoid private coupling."""

---

[omsherikar]

@yuvimittal Please review the changes

[yuvimittal]

@omsherikar LGTM, please push empty commit to check CI/CD

[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

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.', 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

tests/test_llvmlite_helpers.py

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.', 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

[omsherikar]

@omsherikar LGTM, please push empty commit to check CI/CD

@yuvimittal All checks passed

[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

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.', 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

tests/test_llvmlite_helpers.py

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.', 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

[yuvimittal]

LGTM! @omsherikar thanks for working on that