[PATCH] AMDGPU: Verify instructions in non-debug builds as well

[Marek Olšák via llvm-commits]

On Wed, Feb 10, 2016 at 7:57 AM, Michel Dänzer
<llvm-commits at lists.llvm.org> wrote:
> On 10.02.2016 12:17, Michel Dänzer via llvm-commits wrote:
>> From: Michel Dänzer <michel.daenzer at amd.com>
>>
>> And emit an error if it fails.
>>
>> This prevents illegal instructions from getting sent to the GPU, which
>> would potentially result in a hang.
>>
>> This is a candidate for the stable branch(es).
>>
>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
>> ---
>>  lib/Target/AMDGPU/AMDGPUMCInstLower.cpp | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp b/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
>> index dfc652f..929cf87 100644
>> --- a/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
>> +++ b/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
>> @@ -88,13 +88,13 @@ void AMDGPUAsmPrinter::EmitInstruction(const MachineInstr *MI) {
>>    const AMDGPUSubtarget &STI = MF->getSubtarget<AMDGPUSubtarget>();
>>    AMDGPUMCInstLower MCInstLowering(OutContext, STI);
>>
>> -#ifdef _DEBUG
>>    StringRef Err;
>>    if (!STI.getInstrInfo()->verifyInstruction(MI, Err)) {
>> -    errs() << "Warning: Illegal instruction detected: " << Err << "\n";
>> +    LLVMContext &C = MI->getParent()->getParent()->getFunction()->getContext();
>> +    C.emitError("Illegal instruction detected: " + Err);
>>      MI->dump();
>>    }
>> -#endif
>> +
>>    if (MI->isBundle()) {
>>      const MachineBasicBlock *MBB = MI->getParent();
>>      MachineBasicBlock::const_instr_iterator I = ++MI->getIterator();
>>
>
> Unfortunately, this breaks the lit test test/CodeGen/AMDGPU/fmin_legacy.ll:
>
> test_fmin_legacy_f32:                   ; @test_fmin_legacy_f32
> ; BB#0:
>         s_load_dwordx2 s[4:5], s[0:1], 0x9
>         s_load_dwordx4 s[0:3], s[0:1], 0xd
>         s_mov_b32 s7, 0xf000
>         s_mov_b32 s6, -1
> Warning: Illegal instruction detected: VOP* instruction uses the constant bus more than once
>   %VGPR0<def> = V_MIN_LEGACY_F32_e64 0, %SGPR0, 0, %SGPR1, 0, 0, %EXEC<imp-use>
>         s_waitcnt lgkmcnt(0)
>         v_min_legacy_f32_e64 v0, s0, s1
>         buffer_store_dwordx4 v[0:3], s[4:7], 0
>         s_endpgm
>
> v_min_legacy_f32 only supports one SGPR argument. Where should this be
> enforced?

That's a separate bug. For now, I think the test should just be commented out.

Marek