Re: [Mesa-dev] [PATCH 2/2] [swr] fix index buffers with non-zero indices

2017-02-21 Thread Cherniak, Bruce 
Reviewed-by: Bruce Cherniak 

> On Feb 17, 2017, at 2:30 PM, George Kyriazis  
> wrote:
> 
> Fix issue with index buffers that do not contain a 0 index.  0 index
> can be a non-valid index if the (copied) vertex buffers are a subset of the
> user's (which happens because we only copy the range between min & max).
> Core will use an index passed in from the driver to replace invalid indices.
> 
> Only do this for calls that contain non-zero indices, to minimize performance
> cost.
> ---
> src/gallium/drivers/swr/rasterizer/core/state.h|  1 +
> .../drivers/swr/rasterizer/jitter/fetch_jit.cpp| 60 +++---
> .../drivers/swr/rasterizer/jitter/fetch_jit.h  |  2 +
> src/gallium/drivers/swr/swr_draw.cpp   |  1 +
> src/gallium/drivers/swr/swr_state.cpp  |  4 ++
> 5 files changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/src/gallium/drivers/swr/rasterizer/core/state.h 
> b/src/gallium/drivers/swr/rasterizer/core/state.h
> index 2f3b913..05347dc 100644
> --- a/src/gallium/drivers/swr/rasterizer/core/state.h
> +++ b/src/gallium/drivers/swr/rasterizer/core/state.h
> @@ -524,6 +524,7 @@ struct SWR_VERTEX_BUFFER_STATE
> const uint8_t *pData;
> uint32_t size;
> uint32_t numaNode;
> +uint32_t minVertex; // min vertex (for bounds checking)
> uint32_t maxVertex; // size / pitch.  precalculated value 
> used by fetch shader for OOB checks
> uint32_t partialInboundsSize;   // size % pitch.  precalculated value 
> used by fetch shader for partially OOB vertices
> };
> diff --git a/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp 
> b/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp
> index 901bce6..ffa7605 100644
> --- a/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp
> +++ b/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp
> @@ -309,11 +309,29 @@ void FetchJit::JitLoadVertices(const 
> FETCH_COMPILE_STATE , Value* str
> 
> Value* startVertexOffset = MUL(Z_EXT(startOffset, mInt64Ty), stride);
> 
> +Value *minVertex = NULL;
> +Value *minVertexOffset = NULL;
> +if (fetchState.bPartialVertexBuffer) {
> +// fetch min index for low bounds checking
> +minVertex = GEP(streams, {C(ied.StreamIndex), 
> C(SWR_VERTEX_BUFFER_STATE_minVertex)});
> +minVertex = LOAD(minVertex);
> +if (!fetchState.bDisableIndexOOBCheck) {
> +minVertexOffset = MUL(Z_EXT(minVertex, mInt64Ty), stride);
> +}
> +}
> +
> // Load from the stream.
> for(uint32_t lane = 0; lane < mVWidth; ++lane)
> {
> // Get index
> Value* index = VEXTRACT(vCurIndices, C(lane));
> +
> +if (fetchState.bPartialVertexBuffer) {
> +// clamp below minvertex
> +Value *isBelowMin = ICMP_SLT(index, minVertex);
> +index = SELECT(isBelowMin, minVertex, index);
> +}
> +
> index = Z_EXT(index, mInt64Ty);
> 
> Value*offset = MUL(index, stride);
> @@ -321,10 +339,14 @@ void FetchJit::JitLoadVertices(const 
> FETCH_COMPILE_STATE , Value* str
> offset = ADD(offset, startVertexOffset);
> 
> if (!fetchState.bDisableIndexOOBCheck) {
> -// check for out of bound access, including partial OOB, and 
> mask them to 0
> +// check for out of bound access, including partial OOB, and 
> replace them with minVertex
> Value *endOffset = ADD(offset, C((int64_t)info.Bpp));
> Value *oob = ICMP_ULE(endOffset, size);
> -offset = SELECT(oob, offset, ConstantInt::get(mInt64Ty, 0));
> +if (fetchState.bPartialVertexBuffer) {
> +offset = SELECT(oob, offset, minVertexOffset);
> +} else {
> +offset = SELECT(oob, offset, ConstantInt::get(mInt64Ty, 
> 0));
> +}
> }
> 
> Value*pointer = GEP(stream, offset);
> @@ -732,6 +754,13 @@ void FetchJit::JitGatherVertices(const 
> FETCH_COMPILE_STATE ,
> Value *maxVertex = GEP(streams, {C(ied.StreamIndex), 
> C(SWR_VERTEX_BUFFER_STATE_maxVertex)});
> maxVertex = LOAD(maxVertex);
> 
> +Value *minVertex = NULL;
> +if (fetchState.bPartialVertexBuffer) {
> +// min vertex index for low bounds OOB checking
> +minVertex = GEP(streams, {C(ied.StreamIndex), 
> C(SWR_VERTEX_BUFFER_STATE_minVertex)});
> +minVertex = LOAD(minVertex);
> +}
> +
> Value *vCurIndices;
> Value *startOffset;
> if(ied.InstanceEnable)
> @@ -769,9 +798,16 @@ void FetchJit::JitGatherVertices(const 
> FETCH_COMPILE_STATE ,
> 
> // if we have a start offset, subtract from max vertex. Used for OOB 
> check
> maxVertex = SUB(Z_EXT(maxVertex, mInt64Ty),

[Mesa-dev] [PATCH 2/2] [swr] fix index buffers with non-zero indices

2017-02-17 Thread George Kyriazis 
Fix issue with index buffers that do not contain a 0 index.  0 index
can be a non-valid index if the (copied) vertex buffers are a subset of the
user's (which happens because we only copy the range between min & max).
Core will use an index passed in from the driver to replace invalid indices.

Only do this for calls that contain non-zero indices, to minimize performance
cost.
---
 src/gallium/drivers/swr/rasterizer/core/state.h|  1 +
 .../drivers/swr/rasterizer/jitter/fetch_jit.cpp| 60 +++---
 .../drivers/swr/rasterizer/jitter/fetch_jit.h  |  2 +
 src/gallium/drivers/swr/swr_draw.cpp   |  1 +
 src/gallium/drivers/swr/swr_state.cpp  |  4 ++
 5 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/src/gallium/drivers/swr/rasterizer/core/state.h 
b/src/gallium/drivers/swr/rasterizer/core/state.h
index 2f3b913..05347dc 100644
--- a/src/gallium/drivers/swr/rasterizer/core/state.h
+++ b/src/gallium/drivers/swr/rasterizer/core/state.h
@@ -524,6 +524,7 @@ struct SWR_VERTEX_BUFFER_STATE
 const uint8_t *pData;
 uint32_t size;
 uint32_t numaNode;
+uint32_t minVertex; // min vertex (for bounds checking)
 uint32_t maxVertex; // size / pitch.  precalculated value used 
by fetch shader for OOB checks
 uint32_t partialInboundsSize;   // size % pitch.  precalculated value used 
by fetch shader for partially OOB vertices
 };
diff --git a/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp 
b/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp
index 901bce6..ffa7605 100644
--- a/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp
+++ b/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp
@@ -309,11 +309,29 @@ void FetchJit::JitLoadVertices(const FETCH_COMPILE_STATE 
, Value* str
 
 Value* startVertexOffset = MUL(Z_EXT(startOffset, mInt64Ty), stride);
 
+Value *minVertex = NULL;
+Value *minVertexOffset = NULL;
+if (fetchState.bPartialVertexBuffer) {
+// fetch min index for low bounds checking
+minVertex = GEP(streams, {C(ied.StreamIndex), 
C(SWR_VERTEX_BUFFER_STATE_minVertex)});
+minVertex = LOAD(minVertex);
+if (!fetchState.bDisableIndexOOBCheck) {
+minVertexOffset = MUL(Z_EXT(minVertex, mInt64Ty), stride);
+}
+}
+
 // Load from the stream.
 for(uint32_t lane = 0; lane < mVWidth; ++lane)
 {
 // Get index
 Value* index = VEXTRACT(vCurIndices, C(lane));
+
+if (fetchState.bPartialVertexBuffer) {
+// clamp below minvertex
+Value *isBelowMin = ICMP_SLT(index, minVertex);
+index = SELECT(isBelowMin, minVertex, index);
+}
+
 index = Z_EXT(index, mInt64Ty);
 
 Value*offset = MUL(index, stride);
@@ -321,10 +339,14 @@ void FetchJit::JitLoadVertices(const FETCH_COMPILE_STATE 
, Value* str
 offset = ADD(offset, startVertexOffset);
 
 if (!fetchState.bDisableIndexOOBCheck) {
-// check for out of bound access, including partial OOB, and 
mask them to 0
+// check for out of bound access, including partial OOB, and 
replace them with minVertex
 Value *endOffset = ADD(offset, C((int64_t)info.Bpp));
 Value *oob = ICMP_ULE(endOffset, size);
-offset = SELECT(oob, offset, ConstantInt::get(mInt64Ty, 0));
+if (fetchState.bPartialVertexBuffer) {
+offset = SELECT(oob, offset, minVertexOffset);
+} else {
+offset = SELECT(oob, offset, ConstantInt::get(mInt64Ty, 
0));
+}
 }
 
 Value*pointer = GEP(stream, offset);
@@ -732,6 +754,13 @@ void FetchJit::JitGatherVertices(const FETCH_COMPILE_STATE 
,
 Value *maxVertex = GEP(streams, {C(ied.StreamIndex), 
C(SWR_VERTEX_BUFFER_STATE_maxVertex)});
 maxVertex = LOAD(maxVertex);
 
+Value *minVertex = NULL;
+if (fetchState.bPartialVertexBuffer) {
+// min vertex index for low bounds OOB checking
+minVertex = GEP(streams, {C(ied.StreamIndex), 
C(SWR_VERTEX_BUFFER_STATE_minVertex)});
+minVertex = LOAD(minVertex);
+}
+
 Value *vCurIndices;
 Value *startOffset;
 if(ied.InstanceEnable)
@@ -769,9 +798,16 @@ void FetchJit::JitGatherVertices(const FETCH_COMPILE_STATE 
,
 
 // if we have a start offset, subtract from max vertex. Used for OOB 
check
 maxVertex = SUB(Z_EXT(maxVertex, mInt64Ty), Z_EXT(startOffset, 
mInt64Ty));
-Value* neg = ICMP_SLT(maxVertex, C((int64_t)0));
+Value* maxNeg = ICMP_SLT(maxVertex, C((int64_t)0));
 // if we have a negative value, we're already OOB. clamp at 0.
-maxVertex = SELECT(neg, C(0), TRUNC(maxVertex, mInt32Ty));
+maxVertex = SELECT(maxNeg,