R600/SI: Prettier display of input modifiers and fold of fabs

[Daenzer, Michel]

On Mon, 2014-02-10 at 07:21 -0800, Vincent Lejeune wrote:
> 
> I reworked an old patch that improved the dump of SI input modifiers.
> Previous patch was adding 6 input modifiers which made instruction
> manipulation quite difficult.
> 
> 
> This new patch is only adding three (one modifier per inputs).

Nice, I have some minor comments on this patch below though.


> Besides, I also worked on fabs folding (which may solve an issue
> raised lately because of fneg 0.0 issue).

What issue is that?


> Both were tested on hd 7750 with a week old piglit and there was no
> regression.

Did piglit run all tests up to and including OpenGL 3.3 / GLSL 3.30?


> From: Vincent Lejeune <vljn at ovi.com>
> Date: Wed, 5 Feb 2014 17:14:50 +0100
> Subject: [PATCH 2/2] R600/SI: Fold fabs into src input modifier
[...]
> diff --git a/test/CodeGen/R600/fabs.ll b/test/CodeGen/R600/fabs.ll
> index 2cd3a4f..15b7e22 100644
> --- a/test/CodeGen/R600/fabs.ll
> +++ b/test/CodeGen/R600/fabs.ll
> @@ -49,6 +49,16 @@ entry:
>    ret void
>  }
>  
> +; SI-CHECK-LABEL: @fabs_fold
> +; SI-CHECK-NOT: V_AND_B32_e32

This should explicitly check that V_MUL_F32_e64 is used with the correct
modifier value.

> +define void @fabs_fold(float addrspace(1)* %out, float %in0, float %
> in1) {
> +entry:
> +  %0 = call float @fabs(float %in0)
> +  %1 = fmul float %0, %in1
> +  store float %1, float addrspace(1)* %out
> +  ret void
> +}
> +

Other than that, this patch looks good to me.


> From: Vincent Lejeune <vljn at ovi.com>
> Date: Sun, 2 Feb 2014 23:26:49 +0100
> Subject: [PATCH 1/2] R600/SI: Prettier display of input modifiers
[...]
> diff --git a/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp
> b/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp
> index 7105879..e1da2ad 100644
> --- a/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp
> +++ b/lib/Target/R600/InstPrinter/AMDGPUInstPrinter.cpp
> @@ -106,6 +106,18 @@ void AMDGPUInstPrinter::printOperand(const MCInst
> *MI, unsigned OpNo,
>    }
>  }
>  
> +void AMDGPUInstPrinter::printOperandAndMods(const MCInst *MI,
> unsigned OpNo,
> +                                            raw_ostream &O) {
> +  unsigned InputModifiers = MI->getOperand(OpNo).getImm();
> +  if (InputModifiers & 0x1)
> +    O << "- ";

Why a space after the minus sign?


> diff --git a/lib/Target/R600/SIInstructions.td
> b/lib/Target/R600/SIInstructions.td
> index 25fd7d5..fb49e9a 100644
> --- a/lib/Target/R600/SIInstructions.td
> +++ b/lib/Target/R600/SIInstructions.td
[...]
> @@ -1684,8 +1688,7 @@ def : BitConvert <v16f32, v16i32, VReg_512>;
>  
>  def : Pat <
>    (int_AMDIL_clamp f32:$src, (f32 FP_ZERO), (f32 FP_ONE)),
> -  (V_ADD_F32_e64 $src, (i32 0 /* SRC1 */),
> -   0 /* ABS */, 1 /* CLAMP */, 0 /* OMOD */, 0 /* NEG */)
> +  (V_ADD_F32_e64 $src, (i32 0 /* SRC1 */), 1 /* CLAMP */, 0 /* OMOD
> */)
>  >;

The /* OMOD */ comment needs to be adapted or simply removed.


> diff --git a/test/CodeGen/R600/fneg.ll b/test/CodeGen/R600/fneg.ll
> index f4e6be6..7ad760c 100644
> --- a/test/CodeGen/R600/fneg.ll
> +++ b/test/CodeGen/R600/fneg.ll
> @@ -51,7 +51,7 @@ entry:
>  ; R600-CHECK: -KC0[2].Z
>  ; SI-CHECK-LABEL: @fneg_free
>  ; XXX: We could use V_ADD_F32_e64 with the negate bit here instead.
> -; SI-CHECK: V_SUB_F32_e64 v{{[0-9]}}, 0.000000e+00, s{{[0-9]}}, 0, 0,
> 0, 0
> +; SI-CHECK: V_SUB_F32_e64 v{{[0-9]}}, 0.000000e+00, s{{[0-9]}}, 0, 0

Would it be possible to completely remove the printing of the modifier
arguments separately?


Other than that, this patch looks good to me as well.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer