Skip to content

Keep floor_div(x, 1) for float inputs in noop_elimination - #2762

Open
winklemad wants to merge 2 commits into
apple:mainfrom
winklemad:fix-floordiv-noop-elimination
Open

Keep floor_div(x, 1) for float inputs in noop_elimination#2762
winklemad wants to merge 2 commits into
apple:mainfrom
winklemad:fix-floordiv-noop-elimination

Conversation

@winklemad

Copy link
Copy Markdown
Contributor

Fixes #2761.

Problem

noop_elimination removed floor_div(x, y=1) as an identity, grouped with real_div and pow. But floor_div rounds toward negative infinity, so floor_div(x, 1) == floor(x), which equals x only when x is already integral. For a float input the pass silently dropped the floor and changed the model's numerics — no error, just wrong results (e.g. 1.5 carried through as 1.5 instead of 1.0). The pass is in the common (default) pipeline.

elif op.op_type in {"floor_div", "pow", "real_div"}:
    return _remove_elementwise_binary(op, None, 1)

x / 1 == x and x ** 1 == x hold for all reals; floor_div(x, 1) == floor(x) does not.

Fix

Split floor_div out of the group and remove it only when the input is integer (types.is_int(op.x.dtype)); float inputs keep the op. The integer optimization is preserved.

Tests

test_elementwise_elimination used an fp32 input yet asserted floor_div(x, 1) was removed — it now correctly expects the op to be kept. Added test_floor_div_elimination_by_dtype covering both the integer-removed and float-kept cases. Verified the pass behavior locally (applied common::noop_elimination directly): float keeps floor_div, integer removes it, and reverting the fix reproduces the float miscompilation. The full test module runs under the project's CI (it imports torch/tensorflow, which aren't needed to exercise the pass itself).

noop_elimination removed floor_div(x, y=1) as an identity, grouped with real_div
and pow. But floor_div rounds toward negative infinity, so floor_div(x, 1) ==
floor(x), which equals x only when x is already integral. For a float input the
pass silently dropped the floor and changed the model's numerics (e.g. 1.5 was
carried through as 1.5 instead of 1.0).

Restrict the floor_div no-op to integer inputs; float inputs keep the op, and
the integer optimization is preserved. Updated test_elementwise_elimination (its
fp32 input now correctly keeps floor_div) and added
test_floor_div_elimination_by_dtype covering the integer-removed and float-kept
cases.
],
)
def test_floor_div_elimination_by_dtype(self, dtype, one, expected):
# floor_div(x, 1) == floor(x) is a no-op only when x is integral. It must be

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This unit test doesn't really demonstrate the issue. Could you convert a toy and then show that the prediction/output of the converted program is correct? That model should produce an incorrect result without your fix.

test_floor_div_elimination_by_dtype only checks the op graph. Add
test_floor_div_float_prediction_is_floored, which converts a toy program and
compares the prediction against floor(x) + 1. add(_, 1) keeps the floor_div
interior so noop_elimination is free to remove it: without the dtype guard the
floor_div is eliminated and the model returns x + 1, so this test fails before
the fix and passes after.
@winklemad

Copy link
Copy Markdown
Contributor Author

Thanks @TobyRoseman — added test_floor_div_float_prediction_is_floored in ceeb5aa. It converts a toy program (add(floor_div(x, 1), 1) over an fp32 input with non-integral values like 2.7, -1.3, 4.9) and compares the prediction against floor(x) + 1 via run_compare_builder, on both the neuralnetwork/fp32 and mlprogram/fp16 backends.

The add(_, 1) is a non-eliminable consumer that keeps the floor_div interior, so noop_elimination is free to remove it. Without the dtype guard the floor_div gets eliminated and the model returns x + 1 (e.g. 3.7 for x=2.7) instead of floor(x) + 1 (3.0) — I confirmed the op graph goes from [floor_div, add] kept (with the fix) to [add] (without it), so this test fails before the fix and passes after.

@TobyRoseman

Copy link
Copy Markdown
Collaborator

@winklemad - Thanks for the update. Changes look good.

CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/2712923781

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.

noop_elimination silently drops the floor of floor_div(x, 1) for float inputs

2 participants