Bug 76861 - mid3 generates slow code for constant arguments
Summary: mid3 generates slow code for constant arguments
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: Other All
: medium enhancement
Assignee: Petri Latvala
QA Contact: Intel 3D Bugs Mailing List
URL: http://lists.freedesktop.org/archives...
Whiteboard:
Keywords:
Depends on:
Blocks: i965-perf
  Show dependency treegraph
 
Reported: 2014-03-31 21:34 UTC by Ian Romanick
Modified: 2014-11-05 11:16 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Romanick 2014-03-31 21:34:52 UTC
mid3 (from GL_AMD_shader_trinary_minmax) generate slow code if two of the arguments are constants.  The code

    a = mid3(b, 1, 3);

should generate the same code as

    a = clamp(b, 1, 3);

Instead it generates

    (assign  (xyzw) (var_ref a)  (expression vec4 max (expression vec4 min (var_ref b) (constant vec4 (1.0 1.0 1.0 1.0)) ) (expression vec4 max (expression vec4 min (var_ref b0) (constant vec4 (3.0 3.0 3.0 3.0)) ) (constant vec4 (1.0 1.0 1.0 1.0)) ) ) ) 

I think the correct fix for this is to add an ir_triop_mid opcode and a lowering pass (in lower_instructions.cpp).  The lowering pass would then need to be smart enough to do the right thing when some of the operands are constants.

min3 and max3 may have similar problems.

The mid3 issue is likely to become a bigger problem in the future.  Emil Persson (aka Humus) suggested that PS3 / Xbox1 developers use mid3 as a general purpose clamp during his GDC2014 presentation.  See https://www.youtube.com/watch?v=UJq96XZURHw
Comment 1 Iago Toral 2014-09-15 10:22:51 UTC
I was working on this when I realized that a patch had been submitted to the mailing list for review many months ago:
http://lists.freedesktop.org/archives/mesa-dev/2014-April/058631.html

Petri, you mentioned that you would modify the patch to check for constant arguments in opt_algebraic as suggested by reviewers, but I think you never sent a new patch. Is this work stalled? I could try to complete it if that is the case.
Comment 2 Petri Latvala 2014-09-23 06:58:13 UTC
(In reply to comment #1)
> Petri, you mentioned that you would modify the patch to check for constant
> arguments in opt_algebraic as suggested by reviewers, but I think you never
> sent a new patch. Is this work stalled? I could try to complete it if that
> is the case.

I sent a new patch series with a different approach:

http://lists.freedesktop.org/archives/mesa-dev/2014-July/064222.html

Fixes to that series based on review are pending. Feel free to jump in if you feel like it.
Comment 3 Iago Toral 2014-09-26 13:09:22 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Petri, you mentioned that you would modify the patch to check for constant
> > arguments in opt_algebraic as suggested by reviewers, but I think you never
> > sent a new patch. Is this work stalled? I could try to complete it if that
> > is the case.
> 
> I sent a new patch series with a different approach:
> 
> http://lists.freedesktop.org/archives/mesa-dev/2014-July/064222.html
> 
> Fixes to that series based on review are pending. Feel free to jump in if
> you feel like it.

Cool. I have just sent a new version addressing review feedback:
http://lists.freedesktop.org/archives/mesa-dev/2014-September/068544.html
Comment 4 Petri Latvala 2014-11-05 11:16:15 UTC
Commit fd31628c49c93db59b734cd6875d3cd479a84a73 in git fixes this. Thanks for the finalization, Iago.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.