On Tue, Nov 29, 2022 at 7:10 PM Arthur Cohen <arthur.co...@embecosm.com> wrote: > > Hi Richard, > > (...) > > >>>> + > >>>> + unsigned HOST_WIDE_INT ltype_length > >>>> + = wi::ext (wi::to_offset (TYPE_MAX_VALUE (ltype_domain)) > >>>> + - wi::to_offset (TYPE_MIN_VALUE (ltype_domain)) + 1, > >>> > >>> TYPE_MIN_VALUE is not checked to be constant, also the correct > >>> check would be to use TREE_CODE (..) == INTEGER_CST, in > >>> the GCC middle-end an expression '1 + 2' (a PLUS_EXPR) would > >>> be TREE_CONSTANT but wi::to_offset would ICE. > >>> > >>>> + TYPE_PRECISION (TREE_TYPE (ltype_domain)), > >>>> + TYPE_SIGN (TREE_TYPE (ltype_domain))) > >>>> + .to_uhwi (); > >>> > >>> .to_uhwi will just truncate if the value doesn't fit, the same result as > >>> above is achieved with > >>> > >>> unsigned HOST_WIDE_INT ltype_length > >>> = TREE_INT_CST_LOW (TYPE_MAX_VALUE (..)) > >>> - TREE_INT_CST_LOW (TYPE_MIN_VALUE (...)) + 1; > >>> > >>> so it appears you wanted to be "more correct" here (but if I see > >>> correctly you fail on that attempt)? > >>> > > I've made the changes you proposed and noticed failure on our 32-bit CI. > > I've had a look at the values in detail, and it seems that truncating > was the expected behavior. > > On our 64 bit CI, with a testcase containing an array of zero elements, > we get the following values: > > TREE_INT_CST_LOW(TYPE_MAX_VALUE(...)) = 18446744073709551615; > TREE_INT_CST_LOW(TYPE_MIN_VALUE(...)) = 0; > > Adding 1 to the result of the substraction results in an overflow, > wrapping back to zero. > > With the -m32 flag, we get the following values: > > TREE_INT_CST_LOW(TYPE_MAX_VALUE(...)) = 4294967295; > TREE_INT_CST_LOW(TYPE_MIN_VALUE(...)) = 0; > > The addition of 1 does not overflow the unsigned HOST_WIDE_INT type and > we end up with 4294967296 as the length of our array. > > I am not sure on how to fix this behavior, and whether or not it is the > expected one, nor am I familiar enough with the tree API to reproduce > the original behavior. Any input is welcome. > > In the meantime, I'll revert those changes and probably keep the > existing code in the patches if that's okay with you.
Sure - take my comments as that the code needs comments explaining what it tries to do. Apparently I misunderstood the intent (and still don't get it, but I don't remember the part in detail either). > >>> Overall this part of the rust frontend looks OK. Take the comments as > >>> suggestions (for future > >>> enhancements). > > Which seems to be the case :) > > The v4 of patches, which contains a lot of fixes for the issues you > mentioned, will be sent soon. > > All the best, > > Arthur
