Skip to content

Improve overflow bounds for rational scaling factors by using modular decomposition#676

Open
jmontgomery-aurora wants to merge 4 commits into
aurora-opensource:mainfrom
jmontgomery-aurora:jm/improve_scalebyrelational_overflow
Open

Improve overflow bounds for rational scaling factors by using modular decomposition#676
jmontgomery-aurora wants to merge 4 commits into
aurora-opensource:mainfrom
jmontgomery-aurora:jm/improve_scalebyrelational_overflow

Conversation

@jmontgomery-aurora
Copy link
Copy Markdown

Units variables of integral types with large magnitudes can overflow unnecessarily when applying the scaling factor $$\frac{P \cdot X}{Q}$$.

We can mitigate this for small values of P and Q (which should be most cases) by rewriting X as a quotient and remainder relative to Q, which gives us $$P \cdot \lfloor X / Q \rfloor + \frac{P \cdot (X \pmod Q)}{Q}$$. This eliminates overflow as long as $$P \cdot (Q - 1) \le \text{TypeMax}$$. Factoring out $$GCD(P, Q)$$ gives us an even larger range of non-overflowing factors P and Q.

The practical result is that for reasonably sized factors and most larger factors, the entire type range becomes usable without overflow. There's also some truncation risk changes to accommodate the new operation.

…rge numbers

Instead of applying (P*T)/Q, instead use modular decomposition to compute the
rational without overflow. We can extend this for some values of P,Q beyond the
usual limit by using GCD(P,Q) instead.
Technically, only *some* values of T will overflow and it's overly restrictive
to assert on it when most ratios are small values.
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@chiphogg
Copy link
Copy Markdown
Member

Thanks for this contribution! I haven't looked into it in detail yet, but it'd be exciting to avoid overflow in more cases.

Right now, I am knee deep in implementing #484, which will provide the first support for vectors and matrices for #70. I probably won't have any review bandwidth until that gets done. Looks like there are some test failures anyway, now that I've unblocked the jobs.

While you're waiting for me to be able to review, what's the relationship of this change with the already-planned #453? That change would essentially eliminate unnecessary overflow from conversions with non-trivial rational factors, at the cost of using (or, for 64 bits, synthesizing) double-wide integer types to represent the conversion as a pure multiplication, and then discarding half the bits. I'm asking because I think there's a decent chance that work might obviate this PR.

@jmontgomery-aurora
Copy link
Copy Markdown
Author

Widening is a cleaner strategy if you have a larger native type, but I didn't want to pay the cost of the synthetic division in the internal code where I'm seeing this crop up. However, there's a better fix for that particular issue than changing au, so this is just a demo PR to see what you think of the overall approach.

Looking back through, I see some issues with the min/max template bounds that are probably causing the test failures.

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.

3 participants