Bug 106393

I reverted the bisected patch, and the subsequent patch enabling shaderInt16.  Mesa CI was inoperable with the spinning piglit tests.

(In reply to Mark Janes from comment #1)
> I reverted the bisected patch, and the subsequent patch enabling
> shaderInt16.  Mesa CI was inoperable with the spinning piglit tests.

We had never seen this reported by Jenkins but I can confirm that we can reproduce the problem locally, was this test temporarily disabled or something? or could it be silently skipped due to the long run time? We sent the series to Jenkins prior to landing it in master.

Anyway, we have looked into it and it seems the problem is a MOV with a Byte type:

mov(8)          g127<1>UB       g2<32,8,4>UB

These need special handling in the compiler, Chema had already found about this while adding support for 8-bit integers, we just weren't aware that the driver already had a case where Byte types were already being used... I'll talk to Chema about this but we probably already have the fix available.

A series that fixes this is available for review here:
https://lists.freedesktop.org/archives/mesa-dev/2018-May/194049.html

Unfortunately, this behavior reproduced intermittently.  By chance, your test runs did not trigger it.  When it was pushed to master, it occurred on a handful of runs over the course of a day.  Unfortunately, the result was that machines were taken off line (or *fortunately*, because the issue was easy to spot).

It was easy to reproduce when I was bisecting, so I'm not sure why it passed at all in CI.

At any rate, the Igalia devs did everything right with their testing of this series.

Fixed in master with:

commit 5a12bdac09496e00b746421f4c10d77f9b7a8e66
Author: Iago Toral Quiroga <itoral@igalia.com>
Date:   Fri May 4 11:33:07 2018 +0200

    i965/compiler: handle conversion to smaller type in the lowering pass for that
    
    This rollbacks the revert of this same patch introduced in
    commit 7b9c15628aae8729118b648f5f473e6ac926b99b.
    
    And also squahes the following patch to prevent a piglit regression caused
    by this change:
    
    intel/compiler: Fix lower_conversions for 8-bit types.
    Author: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
    
    For 8-bit types the execution type is word. A byte raw MOV has 16-bit
    execution type and 8-bit destination and it shouldn't be considered
    a conversion case. So there is no need to change alignment and enter
    in lower_conversions for these instructions.
    
    Fixes a regresion in the piglit test "glsl-fs-shader-stencil-export"
    that is introduced with this patch from the Vulkan shaderInt16 series:
    'i965/compiler: handle conversion to smaller type in the lowering
    pass for that'. The problem is caused because there is already a case
    in the driver that injects Byte instructions like this:
    
    mov(8)          g127<1>UB       g2<32,8,4>UB
    
    And the aforementioned pass was not accounting for the special
    handling of the execution size of Byte instructions. This patch
    fixes this.
    
    v2: (Jason Ekstrand)
       - Simplify is_byte_raw_mov, include reference to PRM and not
       consider B <-> UB conversions as raw movs.
    
    v3: (Matt Turner)
       - Indentation style fixes.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106393
    Tested-by: Mark Janes <mark.a.janes@intel.com>
    Reviewed-by: Matt Turner <mattst88@gmail.com>
    Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>