Page MenuHomePhabricator

[LSR] Check for signed overflow in NarrowSearchSpaceByDetectingSupersets.
ClosedPublic

Authored by fhahn on Mon, Mar 11, 8:00 AM.

Details

Summary

We are adding a sign extended IR value to an int64_t, which can cause
signed overflows, as in the attached test case, where we have a formula
with BaseOffset = -1 and a constant with numeric_limits<int64_t>::min().

If the addition would overflow, skip the simplification for this
formula. Note that the target triple is required to trigger the failure.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Mon, Mar 11, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 11, 8:00 AM

Looks like at other places we cast one operand to uint64_t and then assign the result to int64_t. Given that the operations reflect formulas based on IR types, I guess that's valid? The LangRef does not explicitly mention the behavior of signed overflow for add & co, but given that it is using 2's complement, it should be defined, right?

It looks like LSR doesn't deal with this sort of issue consistently... some places we try to overflow-check arithmetic on the offset, and others we use wrapping arithmetic.

What should happen, really, is that each formula includes a bitwidth, and all arithmetic involving that formula use arithmetic that wraps at that bitwidth (like an APInt). Otherwise it's not really clear what the computation actually means.

It looks like LSR doesn't deal with this sort of issue consistently... some places we try to overflow-check arithmetic on the offset, and others we use wrapping arithmetic.

What should happen, really, is that each formula includes a bitwidth, and all arithmetic involving that formula use arithmetic that wraps at that bitwidth (like an APInt). Otherwise it's not really clear what the computation actually means.

Right, that makes sense! I guess that would consistently address the signed overflows and potentially codegen errors by wrong wrapping/truncating. I'm happy to look into that. Unfortunately we are this UBSan failure internally, do you think it would be OK to get this in (either as is or with wrapping arithmetic) until the issue is addressed consistently?

It's probably okay to preserve the current behavior here with an explicit uint64_t cast, for now, maybe with a FIXME.

fhahn updated this revision to Diff 190142.Mon, Mar 11, 1:02 PM

use wrapping arithmetic

This revision is now accepted and ready to land.Mon, Mar 11, 5:20 PM
This revision was automatically updated to reflect the committed changes.