Hi Jakub,
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?
> 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,
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,
bitwise not, libgcc calls, stores with "separate extension", as well
as the input of atomic stores. It would be great if you could help me
simplify the changes based on your original design.
The repeated calling of TARGET_C_BITINT_TYPE_INFO is just trying
to minimize the changes. Now that we are calling it very often, having
the return value cached would be a great idea, and I think the
resulting limit on the ABI flavors to be either extended or
non-extended is not really a issue.
On Tue, May 20, 2025 at 09:04:23AM GMT, Jakub Jelinek wrote:
>
> In the later 2 paragraphs you say they are sign or zero extended depending
> on if it is signed or unsigned type. I hope it is the latter and not the
> former.
>
Maybe the ABI text for the N <= 64 case is a bit vague. My intention is that
a _BitInt(N) where N <= 64 should be first sign or zero extended to the width
of a containing type ("the smallest fundamental integral type that can contain
it") according to its signedness, and then it would be laid out just like
the containing type, which may involve another sign or zero extension if passed
in a wider register.
> In any case, I think for targets which set info->extended_p = true; we
> actually need testsuite coverage to verify it works properly.
> I'd think something like
> #define CHECK(x) \
> do { \
> if ((typeof (x)) -1 < 0) \
> { \
> _BitInt(sizeof (x) * __CHAR_BIT__) __x; \
> __builtin_memcpy (&__x, &(x), sizeof (__x)); \
> if (__x != (x)) \
> __builtin_abort (); \
> } \
> else \
> { \
> unsigned _BitInt(sizeof (x) * __CHAR_BIT__) __x; \
> __builtin_memcpy (&__x, &(x), sizeof (__x)); \
> if (__x != (x)) \
> __builtin_abort (); \
> } \
> } while (0)
> and use the macro on various _BitInt variables, arguments, return values
> after various arithmetic operations (and enable those tests solely on
> the info->extended_p targets, which would be loongarch, most likely s390x
> and arm 32-bit).
>
> Jakub
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).
Perhaps it's better to leave it to target-specific tests?
Yujie
