I'm not sure about the compile-time error, or how to hack it in -- at ast_to_hir time, function calls haven't been inlined yet, so I think this gets really nasty?
Will address it in a follow-up patch though as you suggest. -- Chris On Sun, Oct 6, 2013 at 7:06 AM, Kenneth Graunke <kenn...@whitecape.org> wrote: > On 10/05/2013 03:38 AM, Chris Forbes wrote: >> - For HSW: Select the channel based on the component selected (swizzle >> is done in HW) >> - For IVB: Select the channel based on the swizzle state for the >> component selected. Only apply the RG32F w/a if we actually want >> green -- we're about to flag it regardless of swizzle state. >> >> Signed-off-by: Chris Forbes <chr...@ijw.co.nz> >> --- >> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp >> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp >> index 6141009..5508cdc 100644 >> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp >> @@ -1475,7 +1475,8 @@ fs_visitor::visit(ir_texture *ir) >> /* When tg4 is used with the degenerate ZERO/ONE swizzles, don't >> bother >> * emitting anything other than setting up the constant result. >> */ >> - int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0); >> + ir_constant *chan = ir->lod_info.component->as_constant(); >> + int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0]); > > Cool! On my first pass through the spec I hadn't realized that the > channel parameter was required to be a constant integer expression, and > you indeed enforced that by using ir_var_const_in. Makes life here so > much easier. > > There's only one problem: right now, nothing enforces the constraint > that the component parameter must be 0, 1, 2, or 3. Although it isn't > explicitly stated in the spec, I believe specifying another value should > be a compile error. I think this has to get handled at the ast_to_hir > step, and can probably be an ugly special case for now. > > It looks like currently, the bitshifts in GET_SWZ will probably return > 0, resulting in the red channel being selected. So there isn't an out > of bounds access problem...just undefined results. A compile error is > definitely more friendly, though. > > >> if (swiz == SWIZZLE_ZERO || swiz == SWIZZLE_ONE) { >> >> fs_reg res = fs_reg(this, glsl_type::vec4_type); >> @@ -1594,14 +1595,16 @@ fs_visitor::visit(ir_texture *ir) >> uint32_t >> fs_visitor::gather_channel(ir_texture *ir, int sampler) >> { >> - int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0 /* red */); >> - if (c->key.tex.gather_channel_quirk_mask & (1<<sampler)) >> - return 2; /* gather4 sampler is broken for green channel on RG32F -- >> - * we must ask for blue instead. >> - */ >> + ir_constant *chan = ir->lod_info.component->as_constant(); >> + int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0] /* red >> */); > > The /* red */ comment here doesn't make sense anymore...I'd drop it. > >> switch (swiz) { >> case SWIZZLE_X: return 0; >> - case SWIZZLE_Y: return 1; >> + case SWIZZLE_Y: >> + if (c->key.tex.gather_channel_quirk_mask & (1<<sampler)) >> + return 2; /* gather4 sampler is broken for green channel on >> RG32F -- >> + * we must ask for blue instead. >> + */ >> + return 1; > > I would like to see the comment above the block, rather than off to the > side like this. Ditto for the next patch. > >> case SWIZZLE_Z: return 2; >> case SWIZZLE_W: return 3; >> default: >> > > Patches 3-4 are: > Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> > > (since my comment about bounds checking 'comp' would go in a different > patch anyway) _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev