On Tue, May 20, 2025 at 08:58:18PM +0800, Yang Yujie wrote:
> Thanks for the quick review.
>
> Aside from code formatting issues, can I conclude that you suggest
> we should rebase this onto your new big-endian support patch? Or
> do you think it's necessary to add big-endian && extended support
> together?
I'd suggest working on it incrementally rather than with a full patch set.
In one or multiple patches handle the promote_mode stuff, the atomic
extension and expr.cc changes with the feedback incorporated.
For gimple-lower-bitint.cc I'd really like to see what testing you've done
to decide on a case by case basis.
> > Are you sure all those changes were really necessary (rather than doing them
> > just in case)? I believe most of gimple-lower-bitint.cc already should be
> > sign or zero extending the partial limbs when storing stuff, there can be
> > some corner cases (I think one of the shift directions at least).
>
> The modifications to gimple-lower-bitint.cc are based on testing,
The tests weren't included :(.
> since I found that simply setting the "info.extended" flag won't work unless
> I make changes to promote_function_mode, which leads to a series of
> changes to correct all the regtests.
>
> Specifically, the tests told me to extend (thought "truncate"
> was kind of an equivalent word) the output of left shift, plus/minus,
Truncation is the exact opposite of extension.
I can understand the need for handling of left shifts, for all the rest
I'd really like to see testcases.
> More common tests would surely be helpful, especially for new ports.
>
> However, the specific test you mentioned would not be compatible with
> the proposed LoongArch ABI, where the top 64-bit limb within the top
> 128-bit ABI-limb may be undefined. e.g. _BitInt(192).
Ugh, that feels very creative in the psABI :(.
In any case, even that could be handled in the macro, although it would
need to have defined(__loongarch__) specific helpers that would perhaps
for
sizeof (x) * __CHAR_BIT__ >= 128
&& (__builtin_popcountg (~(typeof (x)) 0) & 64) == 0
choose unsigned _BitInt smaller by 64 bits from the one mentioned
in the macro (for signed similarly using __builtin_clrsbg).
> Perhaps it's better to leave it to target-specific tests?
Please don't, we don't want to repeat that for all the info->extended
targets (which looks to be arm 32-bit, s390x and loongarch right now).
We want to test it on all, like the whole bitint testsuite helps to find
issues on all the arches, most of it isn't target specific.
Jakub
