AMDGPU: Skip debug instructions in assert

[arsenm]

Summary

These are inserted after branch relaxation, and for some reason it's
decided to put them in the long branch expansion block. It's probably
not great to rely on the source block address, so this should probably
be switched to being PC relative instead of relying on the block
address

[Orlando]

Are the function attributes in the test necessary? Watching other reviews leads me to believe these are undesirable. Apart from this it LGTMy untrained eyes (I'm new to reviewing!).

[arsenm]

In D60858#1471444, @Orlando wrote:

Are the function attributes in the test necessary? Watching other reviews leads me to believe these are undesirable. Apart from this it LGTMy untrained eyes (I'm new to reviewing!).

They're part of the intrinsic declarations, so they're really there either way. I usually leave the ones part of the intrinsic definition

[echristo]

FWIW they're fine IMO too :)

Testcase and match seem a little long - any chance of shrinking either?

[arsenm]

In D60858#1471490, @echristo wrote:

FWIW they're fine IMO too :)

Testcase and match seem a little long - any chance of shrinking either?

This is as far as I was able to get for an IR test. Any further probably requires a MIR testcase. However, the MIR printer and parser don’t correctly handle debug info references in DBG_VALUE. I discovered recently it mostly works if you strip out all the operands to them, but I’m not sure that will work here. I assume the pass reinserting these actually needs the operands to get the same position

[arsenm]

I could do a MIR test that only runs the asm printer, which will be less useful whenever the long branch expansion is fixed

[echristo]

Looks ok to me. Are you worried about the correctness of the code? That seems to be a lot of matching?

[arsenm]

In D60858#1472184, @echristo wrote:

Looks ok to me. Are you worried about the correctness of the code? That seems to be a lot of matching?

It's just the autogenerated checks, which check everything. I'm worried about something changing whenever I get around to fixing the long branch expansion, and matching all of the long branch blocks is pretty annoying.

[echristo]

Sure. I'd have probably just checked for the function and maybe an instruction or two rather than the whole thing. It's more fragile that way and harder to understand what you're actually checking for since you mostly just said "doesn't crash" :)

[arsenm]

Reduce test a bit

[kzhuravl]

LGTM, thx

[arsenm]

r358909

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

 
 const MCExpr *AMDGPUMCInstLower::getLongBranchBlockExpr(
   const MachineBasicBlock &SrcBB,
   const MachineOperand &MO) const {
   const MCExpr *DestBBSym
     = MCSymbolRefExpr::create(MO.getMBB()->getSymbol(), Ctx);
   const MCExpr *SrcBBSym = MCSymbolRefExpr::create(SrcBB.getSymbol(), Ctx);
 
-  assert(SrcBB.front().getOpcode() == AMDGPU::S_GETPC_B64 &&
-         ST.getInstrInfo()->get(AMDGPU::S_GETPC_B64).Size == 4);
+  // FIXME: The first half of this assert should be removed. This should
+  // probably be PC relative instead of using the source block symbol, and
+  // therefore the indirect branch expansion should use a bundle.
+  assert(
+      skipDebugInstructionsForward(SrcBB.begin(), SrcBB.end())->getOpcode() ==
+          AMDGPU::S_GETPC_B64 &&
 
   // s_getpc_b64 returns the address of next instruction.
   const MCConstantExpr *One = MCConstantExpr::create(4, Ctx);
   SrcBBSym = MCBinaryExpr::createAdd(SrcBBSym, One, Ctx);
 
   if (MO.getTargetFlags() == AMDGPU::TF_LONG_BRANCH_FORWARD)
     return MCBinaryExpr::createSub(DestBBSym, SrcBBSym, Ctx);
 

test/CodeGen/AMDGPU/branch-relaxation-debug-info.ll

+; RUN: llc -O0 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -amdgpu-s-branch-bits=4 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+
+; Make sure there is no assertion due to dbg_value instructions
+; present in the block used for the branch expansion.
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+define amdgpu_kernel void @long_branch_dbg_value(float addrspace(1)* nocapture %arg, float %arg1) #1 !dbg !5 {
+; GCN-LABEL: long_branch_dbg_value:
+; GCN:  BB0_4: ; %bb
+; GCN-NEXT:    ;DEBUG_VALUE: test_debug_value:globalptr_arg <- [DW_OP_plus_uconst 12, DW_OP_stack_value]
+; GCN-NEXT:    .loc 1 0 42 is_stmt 0 ; /tmp/test_debug_value.cl:0:42
+; GCN-NEXT:    s_getpc_b64 s{{\[}}[[PC_LO:[0-9]+]]:[[PC_HI:[0-9]+]]{{\]}}
+; GCN-NEXT:    s_add_u32 s[[PC_LO]], s[[PC_LO]], BB0_3-(BB0_4+4)
+; GCN-NEXT:    s_addc_u32 s[[PC_HI]], s[[PC_HI]], 0
+; GCN-NEXT:    s_setpc_b64
+bb:
+  %tmp = fmul float %arg1, %arg1
+  %tmp2 = getelementptr inbounds float, float addrspace(1)* %arg, i64 3
+  call void @llvm.dbg.value(metadata float addrspace(1)* %tmp2, metadata !11, metadata !DIExpression()) #2, !dbg !12
+  store float %tmp, float addrspace(1)* %tmp2, align 4, !dbg !12
+  %tmp3 = fcmp olt float %tmp, 0x3810000000000000
+  br i1 %tmp3, label %bb8, label %bb4
+
+bb4:                                              ; preds = %bb
+  %tmp5 = load volatile float, float addrspace(1)* undef
+  %tmp6 = fcmp oeq float %tmp5, 0x7FF0000000000000
+  br i1 %tmp6, label %bb7, label %bb8
+
+bb7:                                              ; preds = %bb4
+  br label %bb8
+
+bb8:                                              ; preds = %bb7, %bb4, %bb
+  ret void
+}
+
+attributes #0 = { nounwind readnone speculatable }
+attributes #1 = { nounwind writeonly }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.8.0 (trunk 244715) (llvm/trunk 244718)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "/tmp/test_debug_value.cl", directory: "/Users/matt/src/llvm/build_debug")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "test_debug_value", scope: !1, file: !1, line: 1, type: !6, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+!6 = !DISubroutineType(types: !7)
+!7 = !{null, !8}
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !9, size: 64, align: 32)
+!9 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!10 = !{!11}
+!11 = !DILocalVariable(name: "globalptr_arg", arg: 1, scope: !5, file: !1, line: 1, type: !8)
+!12 = !DILocation(line: 1, column: 42, scope: !5)