On Thursday, June 21, 2018 2:59:30 PM PDT Francisco Jerez wrote:
> Kenneth Graunke <kenn...@whitecape.org> writes:
>
> > On Monday, June 11, 2018 7:25:55 PM PDT Francisco Jerez wrote:
> >> This replaces brw_set_message_descriptor() with the composition of
> >> brw_set_desc() and a new inline helper function that packs the common
> >> message descriptor controls into an integer. The goal is to represent
> >> all message descriptors as a 32-bit integer which is written at once
> >> into the instruction, which is more flexible (SENDS anyone?), robust
> >> (see d2eecf0b0b24d203d0f171807681dffd830d54de fixing an issue
> >> ultimately caused by some bits of the extended message descriptor
> >> being left undefined) and future-proof than the current approach of
> >> specifying the individual descriptor fields directly into the
> >> instruction.
> >>
> >> This approach also seems more self-documenting, since it will allow
> >> removing calls to functions with way too many arguments like
> >> brw_set_*_message() and brw_send_indirect_message(), and instead
> >> provide a single descriptor argument constructed from an appropriate
> >> combination of brw_*_desc() helpers.
> >>
> >> Note that because brw_set_message_descriptor() was (conditionally?)
> >> overriding fields of the instruction which strictly speaking weren't
> >> part of the message descriptor, this involves calling
> >> brw_inst_set_sfid() and brw_inst_set_eot() in some cases in addition
> >> to brw_set_desc().
> >> ---
> >> src/intel/compiler/brw_eu.h | 29 +++++---
> >> src/intel/compiler/brw_eu_emit.c | 108
> >> +++++++++++-------------------
> >> src/intel/compiler/brw_vec4_generator.cpp | 17 +++--
> >> 3 files changed, 68 insertions(+), 86 deletions(-)
> >>
> >> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
> >> index 5a396339fde..b2b20713e45 100644
> >> --- a/src/intel/compiler/brw_eu.h
> >> +++ b/src/intel/compiler/brw_eu.h
> >> @@ -256,14 +256,6 @@ void brw_set_sampler_message(struct brw_codegen *p,
> >> unsigned simd_mode,
> >> unsigned return_format);
> >>
> >> -void brw_set_message_descriptor(struct brw_codegen *p,
> >> - brw_inst *inst,
> >> - enum brw_message_target sfid,
> >> - unsigned msg_length,
> >> - unsigned response_length,
> >> - bool header_present,
> >> - bool end_of_thread);
> >> -
> >> void brw_set_dp_read_message(struct brw_codegen *p,
> >> brw_inst *insn,
> >> unsigned binding_table_index,
> >> @@ -287,6 +279,27 @@ void brw_set_dp_write_message(struct brw_codegen *p,
> >> unsigned end_of_thread,
> >> unsigned send_commit_msg);
> >>
> >> +/**
> >> + * Construct a message descriptor immediate with the specified common
> >> + * descriptor controls.
> >> + */
> >> +static inline uint32_t
> >> +brw_message_desc(const struct gen_device_info *devinfo,
> >> + unsigned msg_length,
> >> + unsigned response_length,
> >> + bool header_present)
> >> +{
> >
> > Perhaps it would be good to add
> >
> > assert(msg_length >= 1 && msg_length <= 15);
> >
> >> + if (devinfo->gen >= 5) {
> >
> > assert(response_length <= 16);
> >
> >
> >> + return (msg_length << 25 |
> >> + response_length << 20 |
> >> + header_present << 19);
> >> + } else {
> >
> > assert(response_length <= 8);
> >
> > I'm not so concerned with validating the values here, just thinking it
> > might make sense to verify that mlen fits in a U4, for example, so we
> > don't accidentally bleed over into other fields when encoding it.
> >
>
> It's kind of a PITA to assert that each field is in range manually for
> each one of these helpers (the following patches introduce a pile of
> functions very much like this one), and verifying that the assertions
> are complete and match the definition of the hardware fields is not
> straightforward to review. I would have used the SET_FIELD() macro if
> it wasn't because it relies on macros being defined with specific
> suffixes for each field. If you believe it's going to be valuable I
> think I'm going to introduce a new helper more easily reusable than
> SET_FIELD() checking for overflow based on a bitfield specification, and
> use it instead of the plain left-shift operators.
I'm fine with landing things as is, but I do think it would be valuable
to assert that the values are in range. At the very least, brw_inst has
historically done that, and genxml's similar asserts have caught all
kinds of problems. I definitely agree that doing this ad-hoc gets
messy, and adding a new macro would help a lot.
You could combine it with the shift, or else just do:
ASSERT_FITS_IN_BITS(val, bits) assert((val & ~((1 << bits) - 1)) == 0)
and then ASSERT_FITS_IN_BITS(msg_length, 5). Depends whether you want
to think of it as "fits in bits 64:60" or "is a 5-bit unsigned value".
Up to you :)
>
> > I suppose the validator already catches this, though...
> >
>
> I don't think the validator will be able to catch such cases in general.
Right...it should catch things like "mlen 0 is invalid", but
can't/shouldn't check bitwidths for every field.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
