[2/2] radv: ensure export arguments are always float

Submitted by Rhys Perry on Dec. 6, 2018, 1:15 p.m.

Details

Message ID 20181206131558.30438-2-pendingchaos02@gmail.com
State Accepted
Commit 0ca550e01ac55c67c2deef50f5cb750a0181352b
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Browsing this patch as part of:
"Series without cover letter" rev 1 in Mesa
<< prev patch [2/2] next patch >>

Commit Message

Rhys Perry Dec. 6, 2018, 1:15 p.m.
So that the signature is correct and consistent, the inputs to a export
intrinsic should always be 32-bit floats.

This and the previous commit fixes a large amount crashes from
dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_*
tests

Fixes: b722b29f10d ('radv: add support for 16bit input/output')
Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
---
 src/amd/vulkan/radv_nir_to_llvm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
index 0c91118e5a..90bcc8dbfe 100644
--- a/src/amd/vulkan/radv_nir_to_llvm.c
+++ b/src/amd/vulkan/radv_nir_to_llvm.c
@@ -2464,12 +2464,8 @@  si_llvm_init_export_args(struct radv_shader_context *ctx,
 	} else
 		memcpy(&args->out[0], values, sizeof(values[0]) * 4);
 
-	for (unsigned i = 0; i < 4; ++i) {
-		if (!(args->enabled_channels & (1 << i)))
-			continue;
-
+	for (unsigned i = 0; i < 4; ++i)
 		args->out[i] = ac_to_float(&ctx->ac, args->out[i]);
-	}
 }
 
 static void

Comments

On 12/6/18 2:15 PM, Rhys Perry wrote:
> So that the signature is correct and consistent, the inputs to a export
> intrinsic should always be 32-bit floats.
> 
> This and the previous commit fixes a large amount crashes from
> dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_*
> tests
> 

They don't crash for me? Please explain how to reproduce.

> Fixes: b722b29f10d ('radv: add support for 16bit input/output')
> Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
> ---
>   src/amd/vulkan/radv_nir_to_llvm.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
> index 0c91118e5a..90bcc8dbfe 100644
> --- a/src/amd/vulkan/radv_nir_to_llvm.c
> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
> @@ -2464,12 +2464,8 @@ si_llvm_init_export_args(struct radv_shader_context *ctx,
>   	} else
>   		memcpy(&args->out[0], values, sizeof(values[0]) * 4);
>   
> -	for (unsigned i = 0; i < 4; ++i) {
> -		if (!(args->enabled_channels & (1 << i)))
> -			continue;
> -
> +	for (unsigned i = 0; i < 4; ++i)
>   		args->out[i] = ac_to_float(&ctx->ac, args->out[i]);
> -	}
>   }
>   
>   static void
>
./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_frag
should crash with something like:
deqp-vk: lib/IR/Instructions.cpp:2590: static llvm::CastInst*
llvm::CastInst::Create(llvm::Instruction::CastOps, llvm::Value*,
llvm::Type*, const llvm::Twine&, llvm::Instruction*): Assertion
`castIsValid(op, S, Ty) && "Invalid cast!"' failed.
because it's trying to zext/sext a half float to a i32.

and ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_vert
should crash with something like:
deqp-vk: lib/IR/Instructions.cpp:348: void
llvm::CallInst::init(llvm::FunctionType*, llvm::Value*,
llvm::ArrayRef<llvm::Value*>,
llvm::ArrayRef<llvm::OperandBundleDefT<llvm::Value*> >, const
llvm::Twine&): Assertion `(i >= FTy->getNumParams() ||
FTy->getParamType(i) == Args[i]->getType()) && "Calling a function
with a bad signature!"' failed.
because it's calling the export intrinsic with incorrect argument types.

For both tests, it seems to only assert with LLVM 8 for some reason.
On Thu, 6 Dec 2018 at 13:31, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
>
>
>
> On 12/6/18 2:15 PM, Rhys Perry wrote:
> > So that the signature is correct and consistent, the inputs to a export
> > intrinsic should always be 32-bit floats.
> >
> > This and the previous commit fixes a large amount crashes from
> > dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_*
> > tests
> >
>
> They don't crash for me? Please explain how to reproduce.
>
> > Fixes: b722b29f10d ('radv: add support for 16bit input/output')
> > Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
> > ---
> >   src/amd/vulkan/radv_nir_to_llvm.c | 6 +-----
> >   1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
> > index 0c91118e5a..90bcc8dbfe 100644
> > --- a/src/amd/vulkan/radv_nir_to_llvm.c
> > +++ b/src/amd/vulkan/radv_nir_to_llvm.c
> > @@ -2464,12 +2464,8 @@ si_llvm_init_export_args(struct radv_shader_context *ctx,
> >       } else
> >               memcpy(&args->out[0], values, sizeof(values[0]) * 4);
> >
> > -     for (unsigned i = 0; i < 4; ++i) {
> > -             if (!(args->enabled_channels & (1 << i)))
> > -                     continue;
> > -
> > +     for (unsigned i = 0; i < 4; ++i)
> >               args->out[i] = ac_to_float(&ctx->ac, args->out[i]);
> > -     }
> >   }
> >
> >   static void
> >
On 12/6/18 3:18 PM, Rhys Perry wrote:
> ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_frag
> should crash with something like:
> deqp-vk: lib/IR/Instructions.cpp:2590: static llvm::CastInst*
> llvm::CastInst::Create(llvm::Instruction::CastOps, llvm::Value*,
> llvm::Type*, const llvm::Twine&, llvm::Instruction*): Assertion
> `castIsValid(op, S, Ty) && "Invalid cast!"' failed.
> because it's trying to zext/sext a half float to a i32.
> 
> and ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_vert
> should crash with something like:
> deqp-vk: lib/IR/Instructions.cpp:348: void
> llvm::CallInst::init(llvm::FunctionType*, llvm::Value*,
> llvm::ArrayRef<llvm::Value*>,
> llvm::ArrayRef<llvm::OperandBundleDefT<llvm::Value*> >, const
> llvm::Twine&): Assertion `(i >= FTy->getNumParams() ||
> FTy->getParamType(i) == Args[i]->getType()) && "Calling a function
> with a bad signature!"' failed.
> because it's calling the export intrinsic with incorrect argument types.
> 
> For both tests, it seems to only assert with LLVM 8 for some reason.

I guess you use a debug llvm build? Can you figure out what change 
introduces this crash?

> On Thu, 6 Dec 2018 at 13:31, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
>>
>>
>>
>> On 12/6/18 2:15 PM, Rhys Perry wrote:
>>> So that the signature is correct and consistent, the inputs to a export
>>> intrinsic should always be 32-bit floats.
>>>
>>> This and the previous commit fixes a large amount crashes from
>>> dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_*
>>> tests
>>>
>>
>> They don't crash for me? Please explain how to reproduce.
>>
>>> Fixes: b722b29f10d ('radv: add support for 16bit input/output')
>>> Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
>>> ---
>>>    src/amd/vulkan/radv_nir_to_llvm.c | 6 +-----
>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
>>> index 0c91118e5a..90bcc8dbfe 100644
>>> --- a/src/amd/vulkan/radv_nir_to_llvm.c
>>> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
>>> @@ -2464,12 +2464,8 @@ si_llvm_init_export_args(struct radv_shader_context *ctx,
>>>        } else
>>>                memcpy(&args->out[0], values, sizeof(values[0]) * 4);
>>>
>>> -     for (unsigned i = 0; i < 4; ++i) {
>>> -             if (!(args->enabled_channels & (1 << i)))
>>> -                     continue;
>>> -
>>> +     for (unsigned i = 0; i < 4; ++i)
>>>                args->out[i] = ac_to_float(&ctx->ac, args->out[i]);
>>> -     }
>>>    }
>>>
>>>    static void
>>>
Seems my LLVM configuration was messed up and I might have used my
distro's LLVM too.

On Thu, 13 Dec 2018 at 08:38, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
>
>
>
> On 12/6/18 3:18 PM, Rhys Perry wrote:
> > ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_frag
> > should crash with something like:
> > deqp-vk: lib/IR/Instructions.cpp:2590: static llvm::CastInst*
> > llvm::CastInst::Create(llvm::Instruction::CastOps, llvm::Value*,
> > llvm::Type*, const llvm::Twine&, llvm::Instruction*): Assertion
> > `castIsValid(op, S, Ty) && "Invalid cast!"' failed.
> > because it's trying to zext/sext a half float to a i32.
> >
> > and ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_vert
> > should crash with something like:
> > deqp-vk: lib/IR/Instructions.cpp:348: void
> > llvm::CallInst::init(llvm::FunctionType*, llvm::Value*,
> > llvm::ArrayRef<llvm::Value*>,
> > llvm::ArrayRef<llvm::OperandBundleDefT<llvm::Value*> >, const
> > llvm::Twine&): Assertion `(i >= FTy->getNumParams() ||
> > FTy->getParamType(i) == Args[i]->getType()) && "Calling a function
> > with a bad signature!"' failed.
> > because it's calling the export intrinsic with incorrect argument types.
> >
> > For both tests, it seems to only assert with LLVM 8 for some reason.
>
> I guess you use a debug llvm build? Can you figure out what change
> introduces this crash?
>
> > On Thu, 6 Dec 2018 at 13:31, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
> >>
> >>
> >>
> >> On 12/6/18 2:15 PM, Rhys Perry wrote:
> >>> So that the signature is correct and consistent, the inputs to a export
> >>> intrinsic should always be 32-bit floats.
> >>>
> >>> This and the previous commit fixes a large amount crashes from
> >>> dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_*
> >>> tests
> >>>
> >>
> >> They don't crash for me? Please explain how to reproduce.
> >>
> >>> Fixes: b722b29f10d ('radv: add support for 16bit input/output')
> >>> Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
> >>> ---
> >>>    src/amd/vulkan/radv_nir_to_llvm.c | 6 +-----
> >>>    1 file changed, 1 insertion(+), 5 deletions(-)
> >>>
> >>> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
> >>> index 0c91118e5a..90bcc8dbfe 100644
> >>> --- a/src/amd/vulkan/radv_nir_to_llvm.c
> >>> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
> >>> @@ -2464,12 +2464,8 @@ si_llvm_init_export_args(struct radv_shader_context *ctx,
> >>>        } else
> >>>                memcpy(&args->out[0], values, sizeof(values[0]) * 4);
> >>>
> >>> -     for (unsigned i = 0; i < 4; ++i) {
> >>> -             if (!(args->enabled_channels & (1 << i)))
> >>> -                     continue;
> >>> -
> >>> +     for (unsigned i = 0; i < 4; ++i)
> >>>                args->out[i] = ac_to_float(&ctx->ac, args->out[i]);
> >>> -     }
> >>>    }
> >>>
> >>>    static void
> >>>
(accidently sent an incomplete email)

Seems my LLVM configuration was messed up and I might have used my
distro's LLVM too.

LLVM 8 and 7 with a release build passes.

A debug build of 8 (and my messed up builds of 7 and 8 which I thought
were release ones) results in an assert.
On Thu, 13 Dec 2018 at 08:38, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
>
>
>
> On 12/6/18 3:18 PM, Rhys Perry wrote:
> > ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_frag
> > should crash with something like:
> > deqp-vk: lib/IR/Instructions.cpp:2590: static llvm::CastInst*
> > llvm::CastInst::Create(llvm::Instruction::CastOps, llvm::Value*,
> > llvm::Type*, const llvm::Twine&, llvm::Instruction*): Assertion
> > `castIsValid(op, S, Ty) && "Invalid cast!"' failed.
> > because it's trying to zext/sext a half float to a i32.
> >
> > and ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_vert
> > should crash with something like:
> > deqp-vk: lib/IR/Instructions.cpp:348: void
> > llvm::CallInst::init(llvm::FunctionType*, llvm::Value*,
> > llvm::ArrayRef<llvm::Value*>,
> > llvm::ArrayRef<llvm::OperandBundleDefT<llvm::Value*> >, const
> > llvm::Twine&): Assertion `(i >= FTy->getNumParams() ||
> > FTy->getParamType(i) == Args[i]->getType()) && "Calling a function
> > with a bad signature!"' failed.
> > because it's calling the export intrinsic with incorrect argument types.
> >
> > For both tests, it seems to only assert with LLVM 8 for some reason.
>
> I guess you use a debug llvm build? Can you figure out what change
> introduces this crash?
>
> > On Thu, 6 Dec 2018 at 13:31, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
> >>
> >>
> >>
> >> On 12/6/18 2:15 PM, Rhys Perry wrote:
> >>> So that the signature is correct and consistent, the inputs to a export
> >>> intrinsic should always be 32-bit floats.
> >>>
> >>> This and the previous commit fixes a large amount crashes from
> >>> dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_*
> >>> tests
> >>>
> >>
> >> They don't crash for me? Please explain how to reproduce.
> >>
> >>> Fixes: b722b29f10d ('radv: add support for 16bit input/output')
> >>> Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
> >>> ---
> >>>    src/amd/vulkan/radv_nir_to_llvm.c | 6 +-----
> >>>    1 file changed, 1 insertion(+), 5 deletions(-)
> >>>
> >>> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
> >>> index 0c91118e5a..90bcc8dbfe 100644
> >>> --- a/src/amd/vulkan/radv_nir_to_llvm.c
> >>> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
> >>> @@ -2464,12 +2464,8 @@ si_llvm_init_export_args(struct radv_shader_context *ctx,
> >>>        } else
> >>>                memcpy(&args->out[0], values, sizeof(values[0]) * 4);
> >>>
> >>> -     for (unsigned i = 0; i < 4; ++i) {
> >>> -             if (!(args->enabled_channels & (1 << i)))
> >>> -                     continue;
> >>> -
> >>> +     for (unsigned i = 0; i < 4; ++i)
> >>>                args->out[i] = ac_to_float(&ctx->ac, args->out[i]);
> >>> -     }
> >>>    }
> >>>
> >>>    static void
> >>>
On 12/13/18 3:45 PM, Rhys Perry wrote:
> (accidently sent an incomplete email)
> 
> Seems my LLVM configuration was messed up and I might have used my
> distro's LLVM too.
> 
> LLVM 8 and 7 with a release build passes.
> 
> A debug build of 8 (and my messed up builds of 7 and 8 which I thought
> were release ones) results in an assert.

Just tried to reproduce with a LLVM 8 debug build, I don't get an 
assertion. Maybe I messed up my build too?

> On Thu, 13 Dec 2018 at 08:38, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
>>
>>
>>
>> On 12/6/18 3:18 PM, Rhys Perry wrote:
>>> ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_frag
>>> should crash with something like:
>>> deqp-vk: lib/IR/Instructions.cpp:2590: static llvm::CastInst*
>>> llvm::CastInst::Create(llvm::Instruction::CastOps, llvm::Value*,
>>> llvm::Type*, const llvm::Twine&, llvm::Instruction*): Assertion
>>> `castIsValid(op, S, Ty) && "Invalid cast!"' failed.
>>> because it's trying to zext/sext a half float to a i32.
>>>
>>> and ./deqp-vk --deqp-case=dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_32_to_16.scalar_uint0_vert
>>> should crash with something like:
>>> deqp-vk: lib/IR/Instructions.cpp:348: void
>>> llvm::CallInst::init(llvm::FunctionType*, llvm::Value*,
>>> llvm::ArrayRef<llvm::Value*>,
>>> llvm::ArrayRef<llvm::OperandBundleDefT<llvm::Value*> >, const
>>> llvm::Twine&): Assertion `(i >= FTy->getNumParams() ||
>>> FTy->getParamType(i) == Args[i]->getType()) && "Calling a function
>>> with a bad signature!"' failed.
>>> because it's calling the export intrinsic with incorrect argument types.
>>>
>>> For both tests, it seems to only assert with LLVM 8 for some reason.
>>
>> I guess you use a debug llvm build? Can you figure out what change
>> introduces this crash?
>>
>>> On Thu, 6 Dec 2018 at 13:31, Samuel Pitoiset <samuel.pitoiset@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/6/18 2:15 PM, Rhys Perry wrote:
>>>>> So that the signature is correct and consistent, the inputs to a export
>>>>> intrinsic should always be 32-bit floats.
>>>>>
>>>>> This and the previous commit fixes a large amount crashes from
>>>>> dEQP-VK.spirv_assembly.instruction.graphics.16bit_storage.input_output_int_*
>>>>> tests
>>>>>
>>>>
>>>> They don't crash for me? Please explain how to reproduce.
>>>>
>>>>> Fixes: b722b29f10d ('radv: add support for 16bit input/output')
>>>>> Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
>>>>> ---
>>>>>     src/amd/vulkan/radv_nir_to_llvm.c | 6 +-----
>>>>>     1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
>>>>> index 0c91118e5a..90bcc8dbfe 100644
>>>>> --- a/src/amd/vulkan/radv_nir_to_llvm.c
>>>>> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
>>>>> @@ -2464,12 +2464,8 @@ si_llvm_init_export_args(struct radv_shader_context *ctx,
>>>>>         } else
>>>>>                 memcpy(&args->out[0], values, sizeof(values[0]) * 4);
>>>>>
>>>>> -     for (unsigned i = 0; i < 4; ++i) {
>>>>> -             if (!(args->enabled_channels & (1 << i)))
>>>>> -                     continue;
>>>>> -
>>>>> +     for (unsigned i = 0; i < 4; ++i)
>>>>>                 args->out[i] = ac_to_float(&ctx->ac, args->out[i]);
>>>>> -     }
>>>>>     }
>>>>>
>>>>>     static void
>>>>>