Matt Turner <matts...@gmail.com> writes:
> On Fri, Oct 24, 2014 at 1:34 AM, Francisco Jerez <curroje...@riseup.net>
> wrote:
>> Matt Turner <matts...@gmail.com> writes:
>>
>>> ---
>>> src/mesa/drivers/dri/i965/brw_fs.cpp | 20 ++++++++++++++++++++
>>> src/mesa/drivers/dri/i965/brw_fs.h | 1 +
>>> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 3 +++
>>> 3 files changed, 24 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> index 983e8db..ffe0a5b 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> @@ -44,6 +44,7 @@ extern "C" {
>>> #include "brw_context.h"
>>> #include "brw_eu.h"
>>> #include "brw_wm.h"
>>> +#include "brw_packed_float.h"
>>> }
>>> #include "brw_fs.h"
>>> #include "brw_cfg.h"
>>> @@ -581,6 +582,18 @@ fs_reg::fs_reg(uint32_t u)
>>> this->width = 1;
>>> }
>>>
>>> +/** Vector float immediate value constructor. */
>>> +fs_reg::fs_reg(uint8_t vf[4])
>>> +{
>>> + init();
>>> + this->file = IMM;
>>> + this->type = BRW_REGISTER_TYPE_VF;
>>> + this->fixed_hw_reg.dw1.ud = (vf[0] << 0) |
>>> + (vf[1] << 8) |
>>> + (vf[2] << 16) |
>>> + (vf[3] << 24);
>>> +}
>>> +
>>
>> IMHO it would be a lot more convenient to have this function take four
>> float arguments and carry out the conversion to 8-bit here. I suspect
>> that in most cases the arguments of this constructor (and the same goes
>> for src_reg) will be known at compile time so we could just add an
>> assertion to check that the conversion was carried out exactly.
>
> That would work if we're constructing VF immediates from known good
> values, but in cases like in an optimization, you're always going to
> have to check whether the values are representable in VF. If they're
> not, the pass will have to fail.
>
Yeah, but isn't this interface unnecessarily annoying to use in the
cases that fit the "known good values" category? (all uses in your
series and in my previous code that makes use of VF) -- It led you write
code like this:
| + uint8_t vf[4] = {0x0, 0x60, 0x70, 0x78};
|[...]
| + emit(MOV(shift, src_reg(vf)));
...which is pretty hard to decode. Don't you find this much easier to
understand? (even if it means we may have to introduce a second
constructor at some point in the future)
| emit(MOV(shift, src_reg(0, 8, 16, 24)));
> In that case, you've already done the conversion. See this pass, for example:
>
> http://cgit.freedesktop.org/~mattst88/mesa/commit/?h=line&id=dd9504e28037aee5ade38bb8b8d51bd39a444e12
pgpnZ7fGyRyrE.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
