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

[Vincent Lejeune]

Hi,

sorry for the delay, I have updated the patches.

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

Yes, there is no issue with GS.

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

I don't understand what you mean.

Vincent

Le Mercredi 12 février 2014 4h36, "Daenzer, Michel" <Michel.Daenzer at amd.com> a écrit :
 
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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140421/79a37414/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-R600-SI-Fold-fabs-fneg-into-src-input-modifier.patch
Type: text/x-patch
Size: 4496 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140421/79a37414/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-R600-SI-Prettier-display-of-input-modifiers.patch
Type: text/x-patch
Size: 12337 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140421/79a37414/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-R600-SI-Use-pseudo-instruction-for-fabs-clamp-fneg.patch
Type: text/x-patch
Size: 4084 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140421/79a37414/attachment-0002.bin>