Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define cast<i32>(u32) overflow behavior #7769

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Conversation

rootjalex
Copy link
Member

The simplifier normalizes reinterpret<i32>(u32) (which has defined behavior) to cast<i32>(u32), which has undefined behavior. Currently, the following code produces inconsistent output:

{
    Expr x = UIntImm::make(UInt(32), 0xFFFFFFF0u);
    Expr e = i32(x + 1);
    e = simplify(e);
    std::cout << e << std::endl;

    e = reinterpret(Int(32), x + 1);
    e = simplify(e);
    std::cout << e << std::endl;
}
{
    Expr x = Variable::make(UInt(32), "x");
    Expr e = i32(x + 1);
    e = simplify(e);
    e = substitute("x", UIntImm::make(UInt(32), 0xFFFFFFF0), e);
    e = simplify(e);
    std::cout << e << std::endl;

    e = reinterpret(Int(32), x + 1);
    e = simplify(e);
    e = substitute("x", UIntImm::make(UInt(32), 0xFFFFFFF0), e);
    e = simplify(e);
    std::cout << e << std::endl;
}

Output:

signed_integer_overflow(0)
-15
signed_integer_overflow(1)
signed_integer_overflow(2)

@abadams and I agreed to simply define cast<i32>(u32) to have the same 2's complement wrapping behavior as reinterpret<i32>(u32). I think the only place this behavior is encoded is in the simplifier (LLVM codegen produces a no-op for either case because LLVM types don't have signedness).

Now the output of the above code is:

-15
-15
-15
-15

@rootjalex rootjalex requested a review from abadams August 16, 2023 21:41
@steven-johnson
Copy link
Contributor

SGTM

@abadams abadams merged commit f2f2af2 into main Aug 17, 2023
3 checks passed
@steven-johnson
Copy link
Contributor

No idea if these are related to this change or not, but:

#7810
#7811

(Just reported by Google-internal fuzz testing after our most recent merge from GitHub)

@abadams abadams added the release_notes For changes that may warrant a note in README for official releases. label Aug 28, 2023
ardier pushed a commit to ardier/Halide-mutation that referenced this pull request Mar 3, 2024
uint32 -> int32 casting should not produce SIO
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_notes For changes that may warrant a note in README for official releases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants