This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix several endless loops in cse_insn for certain target types combinations


Igor Shevlyakov <igor@microunity.com> writes:

> >Another approach would be for you to put
> >
> >INT_MODE (OI, 32);
> >
> >in your CPU-modes.def file, and to patch genmodes.c to add something
> >like
> >    gcc_assert (BITS_PER_WORD < GET_MODE_BITSIZE (MAX_MODE_INT));
> >to init_adjust_machine_modes.
> >
> >The main advantage of this, if it works, would be keeping the code
> >slightly simpler for the ordinary case.
> >
> 
> But that means to introduce fake type, I didn't plan to use. This will
> require to add several new patterns into .md file  (at least movoi),
> handle calling convention for OI type and god knows what else.

No, defining a new mode does not require you to actually handle it
anywhere, any more than the fact that TImode exists by default
requires any backend to handle it.

> It will
> affect the performance in other places since it will take one more
> iteration from MIN_TYPE_INT to MAX_TYPE_INT.

That is true.  But it will only affect your target, not every target.

> Plus, building a cross
> compiler to support 256-bit type will be nightmare. Since part of
> constant optimizations are still performed in HOST_WIDE_INT type, even
> 128-bit support on 32-bit host is broken. I've avoided this problem by
> moving into AMD64 host, but 256-bit will bring that saga back.

This should not be a problem.

> But even in general, to have a code which could loop endlessly and
> pass a knowledge that "in order for compiler to work, you always need
> to define types with 2*BITS_PER_WORD size", looks like not very wise
> maintainance policy to me.

That's why I recommended having init_adjust_machine_modes verify that
the assumptions were met, so nobody else would encounter the problem
which you encountered.

> Oh, by the way, it's not only OI, 256-bit
> floating point mode will also need to be defined.

OK.

> If you are concerned about performance of a loops like that
> 
>       for (wider_mode = GET_MODE_WIDER_MODE (mode);
>            GET_MODE_BITSIZE (wider_mode) <= BITS_PER_WORD;
>            wider_mode = GET_MODE_WIDER_MODE (wider_mode))
> 	{ body; }
> 
> 
> than, first of all, this is not best version either, since it's always
> dereffences memory in condition.

That's true.  I had thought that GET_MODE_BITSIZE referred to a const
array, so the value could be hoisted, but I see that for some reason
mode_size is not const.

> Faster version would be
> 
>       |limit = mode_for_size(BITS_PER_WORD, GET_MODE_CLASS(mode), 0);||
>       for (wider_mode = GET_MODE_WIDER_MODE (mode);
>            wider_mode <= limit;
>            wider_mode = GET_MODE_WIDER_MODE (wider_mode))
> 	{ body; }
> |
> 
> 
> But it still suffers from the problem. The following code will be
> fastest and safe:
> 
>       |limit = mode_for_size(BITS_PER_WORD, GET_MODE_CLASS(mode), 0);
>       wider_mode = mode;
>       do {
>         wider_mode = ||GET_MODE_WIDER_MODE (mode);
>         body;
>       } while (wider_mode != limit};
> 
> |
> 
> Should I redo the patch to use this variant?

Well, I can't approve the patch anyhow.  You should probably see what
an appropriate maintainer thinks.

Ian


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]