On 13/11/2026 08:05, Richard Sandiford wrote:
On Thu, 12 Nov 2026 at 18:38, Alfie Richards <[email protected]> wrote:
On 12/11/2026 05:02, Andrea Pinski wrote:
On Wed, Nov 11, 2026 at 8:56 PM Andrea Pinski
<[email protected]> wrote:
On Wed, Nov 11, 2026 at 9:08 AM Alfie Richards <[email protected]> wrote:
Changes the TARGET_POLY_INT_INDETERMINATE_BOUND to be a macro instead
of a target hook. This is to lower the cost of this operation in
poly_int comparisons which can happen very often.
NAK. We should moving towards target hooks from target macros.
And you didn't add documentation for the new target macro
POLY_INT_INDETERMINATE_BOUND either.
Maybe you could still make a target variable as part of the target
hook like unwind_emit_before_insn is done. Then it is just a load
instead. Would that be ok for performance?
Thanks,
Andrea
Hi Andrea,
Thank you for the review, good to know about not adding macros.
This case is a little awkward as poly_ing.h it is within core types so
tends to get included before the target hooks are included, and loaded
into the gen- files with where they're not included at all, so I will
have to do a little dance to make it work. But that shouldn't be too bad.
I'm much less concerned with performance than I was now I know this
can't be enabled by default, and so will likely be less wide spread.
What you mean by "now I know this can't be enabled by default"?
Was this in response to some other thread, or perhaps to an irc discussion?
At the moment, poly-int.h is an ADT that doesn't even encode
NUM_POLY_INT_COEFFS, which is a more fundamental property than
the bounds of the indeterminates. Using the target bounds seems
like a layering violation to me. (Perhaps that's what you meant
by enabling by default.)
I can see the attraction of having something that handles bounds better,
but I don't think this is the way to go. Even if the argument is that
poly-int.h should become tied to the target, I think that's too big of
a design change to be done at the end of stage 1.
Hi Richard,
Thanks for the review.
I agree with you entirely about the layering issue. I struggled a lot
with getting this to work at all for similar reasons and I was pretty
unhappy with the design. I have a patch in testing that is significantly
more conservative and I think a more natural layering.
The reason we cannot enable this by default is we use this API for two
slightly different things.
- One is to compare the ranges of possible values represented by the
poly-ints at runtime, which this change accomodates.
- The second one is to compare all the coefficients in the poly-int.
Which this breaks. For instance, we may need to check A < B before we do
B - A when A and B are poly-uint64 (to protect against underflow).
So I think this is a really complicated challenge, and will need more
care then initially thought.
Hopefully I will send the other patch today which I think is more
reasonable, and if not then I am happy to delay this to stage 1.
KR,
Alfie
Thanks,
Richard
