LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 33531 - Incorrect PPC64LE VSX code generated for Mesa 17.1.2 vertex shader program
Summary: Incorrect PPC64LE VSX code generated for Mesa 17.1.2 vertex shader program
Status: NEW
Alias: None
Product: libraries
Classification: Unclassified
Component: Backend: PowerPC (show other bugs)
Version: 3.9
Hardware: Other Linux
: P normal
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-20 11:38 PDT by Ben Crocker
Modified: 2017-09-11 10:15 PDT (History)
5 users (show)

See Also:
Fixed By Commit(s):


Attachments
Differences in .shader_test file (10.77 KB, patch)
2017-06-20 12:43 PDT, Ben Crocker
Details
Debug output from Mesa, including LLVM IR (530.39 KB, text/plain)
2017-08-01 12:48 PDT, Ben Crocker
Details
Debugging patch for PPC (2.31 KB, patch)
2017-09-06 21:43 PDT, Tom Stellard
Details
Mesa patch to disable MachineScheduler for ppc64le (1.38 KB, patch)
2017-09-07 19:37 PDT, Tom Stellard
Details
glsl-kwin-blur-1 good assembly from r283190 (231.40 KB, text/plain)
2017-09-08 22:18 PDT, Tom Stellard
Details
glsl-kwin-blur-1 bad assembly from r283188 (885.94 KB, text/plain)
2017-09-08 22:19 PDT, Tom Stellard
Details
glsl-kwin-blur-1 LLVM IR (644.14 KB, text/plain)
2017-09-08 22:19 PDT, Tom Stellard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Crocker 2017-06-20 11:38:35 PDT
Overview:

On PPC64LE, some of the Piglit tests fail because erroneous VSX code is
generated for the vertex shader programs; the same tests pass again if
VSX code generation is disabled (i.e. if just Altivec is used).

Steps to reproduce:

ON YOUR PPC64LE TEST SYSTEM:
0) dnf install mesa-dri-drivers glx-utils redhat-rpm-config
1) Download Mesa 17.1.2 (recent release): git clone -b mesa-17.1.2 git://anongit.freedesktop.org/mesa/mesa
2) dnf builddep mesa
3) Build Mesa: in Mesa directory:
   a) RECOMMEND export CFLAGS="-g -O0" ; eqxport CXXFLAGS="-g -O0"
   b) prefix=<where you want to install Mesa, e.g. $HOME/local/lib>
   c) ./autogen.sh --prefix=$prefix \
	     --with-dri-drivers=  \
	     --with-gallium-drivers=swrast \
	     --enable-gallium-llvm \
             --enable-glx-tls \
             --enable-debug
   d) apply the patch to lp_bld_misc.cpp described below
   e) make
   f) make install
   g) make sure your LD_LIBRARY_PATH points to where 'make install' installed, i.e. to $prefix
4) Back in home directory, download Piglit: git clone git://anongit.freedesktop.org/piglit
5) Install Piglit build dependencies: dnf builddep piglit; IF problems here,
manually install
- cmake
- python-nose
- chrpath
- libtiff-devel
- python2-numpy
- python3-numpy
- libXrender-devel
- python-lxml
- python3-mako
- libpng-devel
- libpng-tools
- opencl-headers
- ocl-icd-devel
- mesa-libGL-devel
- mesa-libGLU-devel
- mesa-libEGL-devel
- mesa-demos glew glew-devel libGLEW
- waffle-devel
- redhat-rpm-config-40-2.fc24.noarch (may not need this after redhat-rpm-config above)
6) In Piglit directory: cmake . ; make
7) Create Piglit results directory (NOT in Piglit directory): mkdir $HOME/piglit-results
8) Run Piglit 'quick' test: in Piglit dir: ./piglit-run.py tests/quick.py
   a) export RESULTDIR=$HOME/piglit-results
   b) export RESULTNAME=q.ppc64le.mesa-17.1.2
   c) ./piglit-run.py tests/quick.py $RESULTDIR/$RESULTNAME
   
ON YOUR PC running your browser, where you should also have Piglit installed:
9) Create Piglit result directory; recommend $HOME/piglit-results
10) Copy the q.ppc64le.mesa-17.1.2 directory over via SCP or other means
11) Create browser-viewable summary:
   a) in Piglit directory...
   b) export RESULTDIR=$HOME/piglit-results
   c) export RESULTNAME=q.ppc64le.mesa-17.1.2
   d) export SUMMARYNAME=sq.ppc64le.mesa-17.1.2
   e) ./piglit summary html $RESULTDIR/$SUMMARYNAME $RESULTDIR/$RESULTNAME

12) Open the summary in a browser tab:
   a) ^O to open a file;
   b) select .../piglit-results/sq.ppc64le.mesa-17.1.2/index.html
   c) narrow the view by selecting "problems"
   d) find the two tests under arb_vertex_attrib_64bit; click on "fail" in the rightmost column
   e) Note multiple color discrepancies: Expected 0 255 0 255, Observed 0 0 255 0


Results: See 12(e) immediately above

Expected Results: No color discrepancy messages; "pass"

Additional info:

Installed on my PPC64LE system:
llvm.ppc64le                    3.9.1-2.fc25           @updates                 
llvm-devel.ppc64le              3.9.1-2.fc25           @updates                 
llvm-libs.ppc64le               3.9.1-2.fc25           @updates

Commands to run the failing tests (in Piglit dir):

bin/shader_runner \
    $HOME/piglit/generated_tests/spec/arb_vertex_attrib_64bit/execution/vs_in/vs-input-float_mat3x4-position-double_dmat4x3.shader_test -auto -fbo 

bin/shader_runner \
    $HOME/piglit/generated_tests/spec/arb_vertex_attrib_64bit/execution/vs_in/vs-input-position-double_dmat3-float_mat2_array3.shader_test -auto -fbo

Enabling/disabling Mesa debug output:

export LIBGL_DEBUG=verbose
export MESA_DEBUG=verbose
export MESA_GLSL=""
export MESA_GLSL="dump log uniform useprog errors"
export GALLIVM_DEBUG="tgsi ir asm dumpbc"
export ST_DEBUG=tgsi

These are very useful when you're debugging Mesa; you will see the
shader program source at several levels, including LLVM IR.
BUT they must all be blank when you're running Piglit, otherwise
the Piglit run will take a VERY long time and the 'summary' step
will not work.

THE TESTS CAN BE MADE TO PASS BY DISABLING VSX code generation; I added
the following patch to lp_bld_misc.cpp to gain finer control over +VSX/-VSX
code generation:

--- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
+++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
@@ -624,7 +624,10 @@ lp_build_create_jit_compiler_for_module(LLVMExecutionEngineRef *OutJIT,
     * See LLVM bug https://llvm.org/bugs/show_bug.cgi?id=26775
     */
    if (util_cpu_caps.has_altivec) {
-      MAttrs.push_back("+vsx");
+      if (getenv("GALLIVM_PPC_NO_VSX"))
+         MAttrs.push_back("-vsx");
+      else
+         MAttrs.push_back("+vsx");
    }
 #endif
 #endif
Comment 1 Ben Crocker 2017-06-20 12:43:00 PDT
Created attachment 18673 [details]
Differences in .shader_test file

P.S.  I wanted to alert you that one of my favorite techniques for
debugging graphics problems--using marked data that stand out in
register dumps--does NOT help in this instance.

For example, I changed 

vs-input-float_mat3x4-position-double_dmat4x3.shader_test
(generated during Piglit build)

as attached and the failing tests passed without disabling
VSX code generation.
Comment 2 Ben Crocker 2017-06-21 09:19:36 PDT
Other Piglit tests that pass with VSX code generation disabled:

Test:	spec@arb_vertex_attrib_64bit@execution@vs_in@vs-input-position-double_dmat3-float_mat2_array3
Command:	bin/shader_runner /root/piglit/generated_tests/spec/arb_vertex_attrib_64bit/execution/vs_in/vs-input-position-double_dmat3-float_mat2_array3.shader_test -auto -fbo 

Test:	shaders@glsl-kwin-blur-1
Command:	bin/glsl-kwin-blur-1 -auto -fbo

Test:	shaders@glsl-kwin-blur-2
Command:	bin/glsl-kwin-blur-2 -auto -fbo
Comment 3 Ben Crocker 2017-06-22 10:39:33 PDT
From the Mesa side, the vertex shader program gets run by code that looks
like this in
<Mesa dir>/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c:

static void
llvm_pipeline_generic(struct draw_pt_middle_end *middle,
                      const struct draw_fetch_info *fetch_info,
                      const struct draw_prim_info *in_prim_info)
{
   ...
   clipped = fpme->current_variant->jit_func(&fpme->llvm->jit_context,
                                             llvm_vert_info.verts,
                                             draw->pt.user.vbuffer,
                                             fetch_info->count,
                                             start_or_maxelt,
                                             fpme->vertex_size,
                                             draw->pt.vertex_buffer,
                                             draw->instance_id,
                                             vid_base,
                                             draw->start_instance,
                                             elts);

   ...
}
Comment 4 Tom Stellard 2017-07-26 10:58:26 PDT
Can you post the LLVM IR for the smallest of the failing tests?
Comment 5 Ben Crocker 2017-08-01 12:48:12 PDT
Created attachment 18889 [details]
Debug output from Mesa, including LLVM IR

Here is the output from a run of Piglit's shader_runner program with
the following debug environmental controls in place:

LIBGL_DEBUG=verbose
MESA_DEBUG=verbose
MESA_GLSL=dump log uniform useprog errors
GALLIVM_DEBUG=tgsi ir asm dumpbc
ST_DEBUG=tgsi

The program is <piglit dir>/bin/shader_runner; the arguments are 
<piglit dir>/generated_tests/spec/arb_vertex_attrib_64bit/execution/vs_in/vs-input-float_mat3x4-position-double_dmat4x3.shader_test -auto

The output is divided into several sections:

1) TGSI source for fragment shader and vertex shader programs
2) GLSL IR for fragment shader and vertex shader programs
3) More TGSI source
4) LLVM IR for fragment shader fs67_variant0_partial
5) LLVM IR for fragment shader fs67_variant0_whole
6) LLC-generated assembly code for fs67_variant0, comprising both
_partial and _whole
7) LLVM IR for setup_variant0
8) LLC-generated assembly code for setup_variant0
9) More TGSI source: the source for the vertex shader program of
interest here
10) LLVM IR for vertex shader draw_llvm_vs_variant0
11) assembly code for ir_draw_llvm_vs_variant0
12) debug output from Mesa
Comment 6 Tom Stellard 2017-08-17 09:12:38 PDT
Can you isolate which shader has the bug?  Maybe by disabling vsx for one shader at a time?
Comment 7 Ben Crocker 2017-08-18 08:41:12 PDT
I've isolated the particular shader program as Tom suggested;
the vertex shader (draw_llvm_vs_variant0) is, in fact, the culprit.
Comment 8 Tom Stellard 2017-09-06 21:43:24 PDT
Created attachment 19116 [details]
Debugging patch for PPC

You could try disabling a few other features via MAttrs:
-direct-move
-power8-vector

Make sure to leave +vsx enabled when you try these.  You won't need to patch LLVM to test these.

In addition, you can apply this patch which adds the following debugging flags to disable some optimizations:

-disable-ppc-lower-vector-shuffle-vsx
-disable-ppc-vsx-swaps

These need to be set using LLVMParseCommandLineOptions()
Comment 9 Tom Stellard 2017-09-07 19:37:36 PDT
Created attachment 19118 [details]
Mesa patch to disable MachineScheduler for ppc64le

I was able to get the tests working by disabling the MachineScheduler on ppc64le. 

I'm still not sure what the root causes is, I'm going to try to see if this is still an issue with trunk.
Comment 10 Nemanja Ivanovic 2017-09-07 22:54:31 PDT
(In reply to Tom Stellard from comment #9)
> Created attachment 19118 [details]
> Mesa patch to disable MachineScheduler for ppc64le
> 
> I was able to get the tests working by disabling the MachineScheduler on
> ppc64le. 
> 
> I'm still not sure what the root causes is, I'm going to try to see if this
> is still an issue with trunk.

Hmm, machine scheduler... I wonder if this is an Anti-dep breaker bug that we saw a while ago where it doesn't handle subregisters correctly. Do the tests pass if you disable AADB?
Although I don't see a way to disable it without modifying:
PPCGenSubtargetInfo::AntiDepBreakMode PPCSubtarget::getAntiDepBreakMode()

to return TargetSubtargetInfo::ANTIDEP_NONE. Mind you I didn't take a super close look at the AADB code.
Comment 11 Nemanja Ivanovic 2017-09-07 22:57:00 PDT
I just realized that the patch meant to fix AADB got stuck in review many moons ago and was effectively abandoned.
https://reviews.llvm.org/D20949

I think that if these test cases pass with AADB turned off, we might want to revive this patch.
Comment 12 Nemanja Ivanovic 2017-09-07 23:04:32 PDT
(In reply to Nemanja Ivanovic from comment #11)
> I just realized that the patch meant to fix AADB got stuck in review many
> moons ago and was effectively abandoned.
> https://reviews.llvm.org/D20949
> 
> I think that if these test cases pass with AADB turned off, we might want to
> revive this patch.

Actually, that's probably not the patch I was thinking of. That one just adds more opportunities for AADB. But I still think it'd be interesting to see if disabling it fixes the problem.

Also, we realized recently that there was a problem in one of the passes not handling aliasing correctly which ended up reordering loads/stores in the scheduler incorrectly. That was fixed in:
https://reviews.llvm.org/rL309651
https://reviews.llvm.org/rL309849

So hopefully as Tom points out, this may not reproduce on trunk.
Comment 13 Ben Crocker 2017-09-08 14:14:41 PDT
I built LLVM 3.9 with help from Tom, installed it, and linked a new build
of Mesa against it.

Of the four Piglit tests I originally identified as failing with VSX
code gen enabled but passing with VSX code gen DISabled:

Test:	spec@arb_vertex_attrib_64bit@execution@vs_in@vs-input-float_mat3x4-position-double_dmat4x3
Command:	bin/shader_runner /root/piglit/generated_tests/spec/arb_vertex_attrib_64bit/execution/vs_in/vs-input-float_mat3x4-position-double_dmat4x3.shader_test -auto -fbo

Test:	spec@arb_vertex_attrib_64bit@execution@vs_in@vs-input-position-double_dmat3-float_mat2_array3
Command:	bin/shader_runner /root/piglit/generated_tests/spec/arb_vertex_attrib_64bit/execution/vs_in/vs-input-position-double_dmat3-float_mat2_array3.shader_test -auto -fbo 

Test:	shaders@glsl-kwin-blur-1
Command:	bin/glsl-kwin-blur-1 -auto -fbo

Test:	shaders@glsl-kwin-blur-2
Command:	bin/glsl-kwin-blur-2 -auto -fbo

the first two now PASS with VSX code gen enabled.

The remaining two, glsl-kwin-blur-1 and glsl-kwin-blur-2, still fail,
so the next thing I'll try will be to use the controls Tom described
in Comment 8.
Comment 14 Ben Crocker 2017-09-08 14:35:35 PDT
Just to clarify:

glsl-kwin-blur-1 and glsl-kwin-blur-2 (still) FAIL with VSX code gen enabled
and PASS with VSX code gen DISabled.
Comment 15 Tom Stellard 2017-09-08 22:15:15 PDT
I just ran git bisect and r283190 was the commit that fixed these tests.  It's not clear if this commit actually fixed the test or just hid the bug by changing the scheduling order.  However, it seems plausible that these failures were caused by a bug sign-extending part words.

I'm attaching the good/bad assembly and the LLVM IR for reference.
Comment 16 Tom Stellard 2017-09-08 22:18:23 PDT
Created attachment 19124 [details]
glsl-kwin-blur-1 good assembly from r283190
Comment 17 Tom Stellard 2017-09-08 22:19:01 PDT
Created attachment 19125 [details]
glsl-kwin-blur-1 bad assembly from r283188
Comment 18 Tom Stellard 2017-09-08 22:19:41 PDT
Created attachment 19126 [details]
glsl-kwin-blur-1 LLVM IR
Comment 19 Tom Stellard 2017-09-11 10:15:19 PDT
For reference, r283190 caused Bug #31127, so if anyone backports this commit they will also need to backport r287765.