[Mesa-dev] [PATCH 14/22] intel/compiler: Do image load/store lowering to NIR
Jason Ekstrand
jason at jlekstrand.net
Wed Aug 29 12:31:38 UTC 2018
On Wed, Aug 29, 2018 at 12:49 AM Kenneth Graunke <kenneth at whitecape.org>
wrote:
> On Friday, August 17, 2018 1:06:20 PM PDT Jason Ekstrand wrote:
> [snip]
> > +# Intel-specific query for loading from the brw_image_param struct
> passed
> > +# into the shader as a uniform. The variable is a deref to the image
> > +# variable. The const index specifies which of the six parameters to
> load.
>
> This might be our first driver-specific intrinsics. Some people make
> big extensibility systems for that, where drivers can extend with their
> own concepts. But given that we all live in the same project, I think
> this makes a lot of sense - just have everybody add their own here,
> suffixed with a name they own (i.e. intel, amd, radv, ir3, whatever).
>
We almost added an ir3 intrinsic some time ago. I don't remember what it
was or why it didn't happen.
> It's certainly nice and simple.
>
> > +intrinsic("image_deref_load_param_intel", src_comp=[1], dest_comp=0,
> > + indices=[BASE], flags=[CAN_ELIMINATE, CAN_REORDER])
> > +intrinsic("image_deref_load_raw_intel", src_comp=[1, 1], dest_comp=0,
> > + flags=[CAN_ELIMINATE, CAN_REORDER])
> > +intrinsic("image_deref_store_raw_intel", src_comp=[1, 1, 0])
>
> I don't think you want CAN_REORDER for the new load intrinsic...at
> least, image_deref_load only has CAN_ELIMINATE. It probably makes
> sense for the two to match...
>
Right... Thanks!
> [snip]
> > +static nir_ssa_def *
> > +image_address(nir_builder *b, const struct gen_device_info *devinfo,
> > + nir_deref_instr *deref, nir_ssa_def *coord)
> > +{
> > + coord = sanitize_image_coord(b, deref, coord);
> > +
> > + nir_ssa_def *offset = load_image_param(b, deref, OFFSET);
> > + nir_ssa_def *tiling = load_image_param(b, deref, TILING);
> > + nir_ssa_def *stride = load_image_param(b, deref, STRIDE);
> > +
> > + /* Shift the coordinates by the fixed surface offset. It may be
> non-zero
> > + * if the image is a single slice of a higher-dimensional surface,
> or if a
> > + * non-zero mipmap level of the surface is bound to the pipeline.
> The
> > + * offset needs to be applied here rather than at surface state
> set-up time
> > + * because the desired slice-level may start mid-tile, so simply
> shifting
> > + * the surface base address wouldn't give a well-formed tiled
> surface in
> > + * the general case.
> > + */
> > + nir_ssa_def *xypos = (coord->num_components == 1) ?
> > + nir_vec2(b, coord, nir_imm_int(b, 0)) :
> > + nir_channels(b, coord, 0x3);
> > + xypos = nir_iadd(b, xypos, offset);
> > +
> > + /* The layout of 3-D textures in memory is sort-of like a tiling
> > + * format. At each miplevel, the slices are arranged in rows of
> > + * 2^level slices per row. The slice row is stored in tmp.y and
> > + * the slice within the row is stored in tmp.x.
> > + *
> > + * The layout of 2-D array textures and cubemaps is much simpler:
> > + * Depending on whether the ARYSPC_LOD0 layout is in use it will be
> > + * stored in memory as an array of slices, each one being a 2-D
> > + * arrangement of miplevels, or as a 2D arrangement of miplevels,
> > + * each one being an array of slices. In either case the separation
> > + * between slices of the same LOD is equal to the qpitch value
> > + * provided as stride.w.
> > + *
> > + * This code can be made to handle either 2D arrays and 3D textures
> > + * by passing in the miplevel as tile.z for 3-D textures and 0 in
> > + * tile.z for 2-D array textures.
> > + *
> > + * See Volume 1 Part 1 of the Gen7 PRM, sections 6.18.4.7 "Surface
> > + * Arrays" and 6.18.6 "3D Surfaces" for a more extensive discussion
> > + * of the hardware 3D texture and 2D array layouts.
> > + */
> > + if (coord->num_components > 2) {
> > + /* Decompose z into a major (tmp.y) and a minor (tmp.x)
> > + * index.
> > + */
> > + nir_ssa_def *z = nir_channel(b, coord, 2);
> > + nir_ssa_def *z_x = nir_ubfe(b, z, nir_imm_int(b, 0),
> > + nir_channel(b, tiling, 2));
> > + nir_ssa_def *z_y = nir_ushr(b, z, nir_channel(b, tiling, 2));
> > +
> > + /* Take into account the horizontal (tmp.x) and vertical (tmp.y)
> > + * slice offset.
> > + */
> > + xypos = nir_iadd(b, xypos, nir_imul(b, nir_vec2(b, z_x, z_y),
> > + nir_channels(b, stride,
> 0xc)));
> > + }
> > +
> > + nir_ssa_def *addr;
> > + if (coord->num_components > 1) {
> > + /* Calculate the major/minor x and y indices. In order to
> > + * accommodate both X and Y tiling, the Y-major tiling format is
> > + * treated as being a bunch of narrow X-tiles placed next to each
> > + * other. This means that the tile width for Y-tiling is actually
> > + * the width of one sub-column of the Y-major tile where each 4K
> > + * tile has 8 512B sub-columns.
> > + *
> > + * The major Y value is the row of tiles in which the pixel lives.
> > + * The major X value is the tile sub-column in which the pixel
> > + * lives; for X tiling, this is the same as the tile column, for Y
> > + * tiling, each tile has 8 sub-columns. The minor X and Y indices
> > + * are the position within the sub-column.
> > + */
> > +
> > + /* Calculate the minor x and y indices. */
> > + nir_ssa_def *minor = nir_ubfe(b, xypos, nir_imm_int(b, 0),
> > + nir_channels(b, tiling, 0x3));
> > + nir_ssa_def *major = nir_ushr(b, xypos, nir_channels(b, tiling,
> 0x3));
> > +
> > + /* Calculate the texel index from the start of the tile row and
> the
> > + * vertical coordinate of the row.
> > + * Equivalent to:
> > + * tmp.x = (major.x << tile.y << tile.x) +
> > + * (minor.y << tile.x) + minor.x
> > + * tmp.y = major.y << tile.y
> > + */
> > + nir_ssa_def *idx_x, *idx_y;
> > + idx_x = nir_ishl(b, nir_channel(b, major, 0), nir_channel(b,
> tiling, 1));
> > + idx_x = nir_iadd(b, idx_x, nir_channel(b, minor, 1));
> > + idx_x = nir_ishl(b, idx_x, nir_channel(b, tiling, 0));
> > + idx_x = nir_iadd(b, idx_x, nir_channel(b, minor, 0));
> > + idx_y = nir_ishl(b, nir_channel(b, major, 1), nir_channel(b,
> tiling, 1));
> > +
> > + /* Add it to the start of the tile row. */
> > + nir_ssa_def *idx;
> > + idx = nir_imul(b, idx_y, nir_channel(b, stride, 1));
> > + idx = nir_iadd(b, idx, idx_x);
> > +
>
> Maybe preserve the /* Multiply by Bpp value */ comment here?
>
Done
> > + addr = nir_imul(b, idx, nir_channel(b, stride, 0));
> > +
> > + if (devinfo->gen < 8 && !devinfo->is_baytrail) {
> > + /* Take into account the two dynamically specified shifts.
> Both need
> > + * are used to implement swizzling of X-tiled surfaces. For
> Y-tiled
>
> May as well fix the "Both need are used" wording here -> "Both are used"
>
Done
> [snip]
> > +static bool
> > +lower_image_load_instr(nir_builder *b,
> > + const struct gen_device_info *devinfo,
> > + nir_intrinsic_instr *intrin)
> > +{
> > + nir_deref_instr *deref = nir_src_as_deref(intrin->src[0]);
> > + nir_variable *var = nir_deref_instr_get_variable(deref);
> > + const enum isl_format image_fmt =
> > + isl_format_for_gl_format(var->data.image.format);
> > +
> > + if (isl_has_matching_typed_storage_image_format(devinfo, image_fmt))
> {
> > + const enum isl_format lower_fmt =
> > + isl_lower_storage_image_format(devinfo, image_fmt);
> > + const unsigned dest_components = intrin->num_components;
> > +
> > + /* Use an undef to hold the uses of the load while we do the color
> > + * conversion.
> > + */
> > + nir_ssa_def *placeholder = nir_ssa_undef(b, 4, 32);
> > + nir_ssa_def_rewrite_uses(&intrin->dest.ssa,
> nir_src_for_ssa(placeholder));
> > +
> > + intrin->num_components = isl_format_get_num_channels(lower_fmt);
> > + intrin->dest.ssa.num_components = intrin->num_components;
> > +
> > + nir_ssa_def *value;
> > + if (devinfo->gen == 7 && !devinfo->is_haswell) {
> > + /* Check the first component of the size field to find out if
> the
> > + * image is bound. Necessary on IVB because it don't seem to
> respect
> > + * null surfaces and will hang when no image is bound.
> > + */
> > + b->cursor = nir_instr_remove(&intrin->instr);
> > + nir_ssa_def *size = load_image_param(b, deref, SIZE);
> > + nir_push_if(b, nir_ine(b, nir_channel(b, size, 0),
> nir_imm_int(b, 0)));
> > + nir_builder_instr_insert(b, &intrin->instr);
> > + nir_push_else(b, NULL);
> > + nir_ssa_def *zero = nir_zero_vec(b, intrin->num_components);
> > + nir_pop_if(b, NULL);
> > + value = nir_if_phi(b, &intrin->dest.ssa, zero);
> > + } else {
> > + b->cursor = nir_after_instr(&intrin->instr);
> > + value = &intrin->dest.ssa;
> > + }
>
> I don't think this Gen7 code is necessary...at least, I don't see the
> old code checking this for typed loads. (Atomics, yes...loads, no...)
>
You're right. It was an extra check on untyped loads and stores which
checked that bpp > 4. Typed was left alone. I've reworked things and have
kicked it off to Jenkins. I'll send a v2 when it comes back.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180829/1f1e8f73/attachment-0001.html>
More information about the mesa-dev
mailing list