Discussion:
[PATCH] Revert "i965/fs: Use align1 mode on ternary instructions on Gen10+"
(too old to reply)
Anuj Phogat
2017-12-22 21:54:08 UTC
Permalink
This reverts commit 9cd60fce9c22737000a8f8dc711141f8a523fe75.
Above commit caused 2000+ piglit tests to assert fail. Disabling
the align1 mode on gen10 for now to avoid failures.

Cc: Matt Turner <***@gmail.com>
Cc: Rafael Antognolli <***@intel.com>
Signed-off-by: Anuj Phogat <***@gmail.com>
---
---
src/intel/compiler/brw_fs_generator.cpp | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/intel/compiler/brw_fs_generator.cpp b/src/intel/compiler/brw_fs_generator.cpp
index 679c1f1916..6a3b2dcf8a 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -1758,15 +1758,13 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)

case BRW_OPCODE_MAD:
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_MAD(p, dst, src[0], src[1], src[2]);
break;

case BRW_OPCODE_LRP:
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_LRP(p, dst, src[0], src[1], src[2]);
break;

@@ -1864,8 +1862,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)

case BRW_OPCODE_BFE:
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFE(p, dst, src[0], src[1], src[2]);
break;

@@ -1875,8 +1872,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
break;
case BRW_OPCODE_BFI2:
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFI2(p, dst, src[0], src[1], src[2]);
break;
--
2.13.3
Rafael Antognolli
2017-12-22 21:57:41 UTC
Permalink
I can confirm this fixes the 2000+ failures.
Post by Anuj Phogat
This reverts commit 9cd60fce9c22737000a8f8dc711141f8a523fe75.
Above commit caused 2000+ piglit tests to assert fail. Disabling
the align1 mode on gen10 for now to avoid failures.
---
---
src/intel/compiler/brw_fs_generator.cpp | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/intel/compiler/brw_fs_generator.cpp b/src/intel/compiler/brw_fs_generator.cpp
index 679c1f1916..6a3b2dcf8a 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -1758,15 +1758,13 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_MAD(p, dst, src[0], src[1], src[2]);
break;
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_LRP(p, dst, src[0], src[1], src[2]);
break;
@@ -1864,8 +1862,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFE(p, dst, src[0], src[1], src[2]);
break;
@@ -1875,8 +1872,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
break;
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFI2(p, dst, src[0], src[1], src[2]);
break;
--
2.13.3
Emil Velikov
2017-12-25 11:05:17 UTC
Permalink
Post by Anuj Phogat
This reverts commit 9cd60fce9c22737000a8f8dc711141f8a523fe75.
Above commit caused 2000+ piglit tests to assert fail. Disabling
the align1 mode on gen10 for now to avoid failures.
Ouch, any ideas why this did not flag up earlier? The commit itself
landed back in Oct.
Should we pick this for stable, since 17.3.x has the offending commit?

Thanks
Emil
Anuj Phogat
2017-12-27 00:43:24 UTC
Permalink
Post by Emil Velikov
Post by Anuj Phogat
This reverts commit 9cd60fce9c22737000a8f8dc711141f8a523fe75.
Above commit caused 2000+ piglit tests to assert fail. Disabling
the align1 mode on gen10 for now to avoid failures.
Ouch, any ideas why this did not flag up earlier? The commit itself
landed back in Oct.
Should we pick this for stable, since 17.3.x has the offending commit?
We still don't have a Cannonlake system plugged in to our CI system.
That's the reason we couldn't catch these regressions.

I posted this patch as a temporary solution to avoid assertion. I
would wait to hear from Matt before picking it to stable.
Post by Emil Velikov
Thanks
Emil
Jason Ekstrand
2017-12-26 15:16:17 UTC
Permalink
What is the assertion? Just shutting off aligna1 ternaries seems a bit
harsh.

--Jason
Post by Anuj Phogat
This reverts commit 9cd60fce9c22737000a8f8dc711141f8a523fe75.
Above commit caused 2000+ piglit tests to assert fail. Disabling
the align1 mode on gen10 for now to avoid failures.
---
---
src/intel/compiler/brw_fs_generator.cpp | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/intel/compiler/brw_fs_generator.cpp
b/src/intel/compiler/brw_fs_generator.cpp
index 679c1f1916..6a3b2dcf8a 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -1758,15 +1758,13 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_MAD(p, dst, src[0], src[1], src[2]);
break;
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_LRP(p, dst, src[0], src[1], src[2]);
break;
@@ -1864,8 +1862,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFE(p, dst, src[0], src[1], src[2]);
break;
@@ -1875,8 +1872,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
break;
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFI2(p, dst, src[0], src[1], src[2]);
break;
--
2.13.3
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Anuj Phogat
2017-12-27 00:53:01 UTC
Permalink
Post by Jason Ekstrand
What is the assertion? Just shutting off aligna1 ternaries seems a bit
harsh.
In brw_eu_emit.c:
assert((src0.vstride == BRW_VERTICAL_STRIDE_0 &&
src0.hstride == BRW_HORIZONTAL_STRIDE_0) ||
(src0.vstride == BRW_VERTICAL_STRIDE_8 &&
src0.hstride == BRW_HORIZONTAL_STRIDE_1));

I agree, but I don't know what's the right fix. I wrote this patch in
hurry just before starting my vacation. Matt can enable it back when
he pushes the fix.
Post by Jason Ekstrand
--Jason
Post by Anuj Phogat
This reverts commit 9cd60fce9c22737000a8f8dc711141f8a523fe75.
Above commit caused 2000+ piglit tests to assert fail. Disabling
the align1 mode on gen10 for now to avoid failures.
---
---
src/intel/compiler/brw_fs_generator.cpp | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/intel/compiler/brw_fs_generator.cpp
b/src/intel/compiler/brw_fs_generator.cpp
index 679c1f1916..6a3b2dcf8a 100644
--- a/src/intel/compiler/brw_fs_generator.cpp
+++ b/src/intel/compiler/brw_fs_generator.cpp
@@ -1758,15 +1758,13 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_MAD(p, dst, src[0], src[1], src[2]);
break;
assert(devinfo->gen >= 6);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_LRP(p, dst, src[0], src[1], src[2]);
break;
@@ -1864,8 +1862,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFE(p, dst, src[0], src[1], src[2]);
break;
@@ -1875,8 +1872,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
break;
assert(devinfo->gen >= 7);
- if (devinfo->gen < 10)
- brw_set_default_access_mode(p, BRW_ALIGN_16);
+ brw_set_default_access_mode(p, BRW_ALIGN_16);
brw_BFI2(p, dst, src[0], src[1], src[2]);
break;
--
2.13.3
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Loading...