AMDGPU: Don't error on calls to null or undef

[arsenm]

Summary

Calls to constants should probably be generally handled.

[rampitec]

What's the point of hiding an error? We should rather produce a normal error message.

[arsenm]

There is no error to hide. This is perfectly valid IR

[rampitec]

In D51794#1228186, @arsenm wrote:

There is no error to hide. This is perfectly valid IR

Could you elaborate how did we end up with a call to undef?

[arsenm]

HCC had a bug recently that was producing calls to under. I assume a call to null would appear for a pure virtual call. They are undefined to execute, but they could appear in dead code that is never reached so it’s not an error to have them exist

[rampitec]

In D51794#1228202, @arsenm wrote:

HCC had a bug recently that was producing calls to under. I assume a call to null would appear for a pure virtual call. They are undefined to execute, but they could appear in dead code that is never reached so it’s not an error to have them exist

A standard reaction to pure virtual call is a runtime error message and abort. We cannot produce an error message from kernel but we can abort. I would suggest to lower it to s_trap.

[arsenm]

In D51794#1228246, @rampitec wrote:

In D51794#1228202, @arsenm wrote:

HCC had a bug recently that was producing calls to under. I assume a call to null would appear for a pure virtual call. They are undefined to execute, but they could appear in dead code that is never reached so it’s not an error to have them exist

A standard reaction to pure virtual call is a runtime error message and abort. We cannot produce an error message from kernel but we can abort. I would suggest to lower it to s_trap.

I don't think this is how that's implemented. Other targets generate a literal call to null in this case. This is an edge case since typically undef/null calls are turned into unreachable in simplifycfg, which there is a flag to generate a trap on separately.

[rampitec]

In D51794#1228453, @arsenm wrote:

In D51794#1228246, @rampitec wrote:

In D51794#1228202, @arsenm wrote:

HCC had a bug recently that was producing calls to under. I assume a call to null would appear for a pure virtual call. They are undefined to execute, but they could appear in dead code that is never reached so it’s not an error to have them exist

A standard reaction to pure virtual call is a runtime error message and abort. We cannot produce an error message from kernel but we can abort. I would suggest to lower it to s_trap.

I don't think this is how that's implemented. Other targets generate a literal call to null in this case. This is an edge case since typically undef/null calls are turned into unreachable in simplifycfg, which there is a flag to generate a trap on separately.

That is actually very inconvenient to debug kernels which simply hang or misbehave, even if that is an UB. I still suggest our action of preference on unreachable code is to trap. Whenever we can or cannot do it consistently for all unreachable does not seem to matter, we need to start somewhere.

[arsenm]

Considering emitting traps requires fixing traps first, otherwise a program that should work will incorrectly trap

[rampitec]

In D51794#1233163, @arsenm wrote:

Considering emitting traps requires fixing traps first, otherwise a program that should work will incorrectly trap

Do you agree that is how we have to handle it in principle? If that requres further fixes I do not object a todo comment here.

[arsenm]

Rebase

[arsenm]

In D51794#1233556, @rampitec wrote:

In D51794#1233163, @arsenm wrote:

Considering emitting traps requires fixing traps first, otherwise a program that should work will incorrectly trap

Do you agree that is how we have to handle it in principle? If that requres further fixes I do not object a todo comment here.

I think this purpose would be better served by some sort of sanitizer pass. These would typically be deleted, and it's an anomaly these would reach codegen to bother trapping on. Something else should have inserted a trap for useful debugging purposes

[rampitec]

I think I've seen calls which should be undef recently, but should stay in a dead code. This is the way our library works, and recently the same behavior seems to appear normal with hcc. It is still a fuzzy logic, but that is way we are handling differences between targets, so let it be.

I would still want to emit a call to an error reporting function though. Although I understand that is difficult to emit a good error from a GPU RT.

[arsenm]

r375356

lib/Target/AMDGPU/SIISelLowering.cpp

SDValue SITargetLowering::LowerCall(CallLoweringInfo &CLI,
   SDValue Callee = CLI.Callee;
   bool &IsTailCall = CLI.IsTailCall;
   CallingConv::ID CallConv = CLI.CallConv;
   bool IsVarArg = CLI.IsVarArg;
   bool IsSibCall = false;
   bool IsThisReturn = false;
   MachineFunction &MF = DAG.getMachineFunction();
 
+  if (Callee.isUndef() || isNullConstant(Callee)) {
+    if (!CLI.IsTailCall) {
+      for (unsigned I = 0, E = CLI.Ins.size(); I != E; ++I)
+        InVals.push_back(DAG.getUNDEF(CLI.Ins[I].VT));
+    }
+
+    return Chain;
+  }
+
   if (IsVarArg) {
     return lowerUnhandledCall(CLI, InVals,
                               "unsupported call to variadic function ");
   }
 
   if (!CLI.CS.getInstruction())
     report_fatal_error("unsupported libcall legalization");
 

test/CodeGen/AMDGPU/call-constant.ll

+; RUN: llc -mtriple=amdgcn-amd-amdhsa < %s | FileCheck -check-prefix=GCN %s
+
+; FIXME: Emitting unnecessary flat_scratch setup
+
+; GCN-LABEL: {{^}}test_call_undef:
+; GCN: s_mov_b32 s8, s7
+; GCN: s_mov_b32 flat_scratch_lo, s5
+; GCN: s_add_u32 s4, s4, s8
+; GCN: s_lshr_b32
+; GCN: s_endpgm
+define amdgpu_kernel void @test_call_undef() #0 {
+  %val = call i32 undef(i32 1)
+  %op = add i32 %val, 1
+  store volatile i32 %op, i32 addrspace(1)* undef
+  ret void
+}
+
+; GCN-LABEL: {{^}}test_tail_call_undef:
+; GCN: s_waitcnt
+; GCN-NEXT: .Lfunc_end
+define i32 @test_tail_call_undef() #0 {
+  %call = tail call i32 undef(i32 1)
+  ret i32 %call
+}
+
+; GCN-LABEL: {{^}}test_call_null:
+; GCN: s_mov_b32 s8, s7
+; GCN: s_mov_b32 flat_scratch_lo, s5
+; GCN: s_add_u32 s4, s4, s8
+; GCN: s_lshr_b32
+; GCN: s_endpgm
+define amdgpu_kernel void @test_call_null() #0 {
+  %val = call i32 null(i32 1)
+  %op = add i32 %val, 1
+  store volatile i32 %op, i32 addrspace(1)* null
+  ret void
+}
+
+; GCN-LABEL: {{^}}test_tail_call_null:
+; GCN: s_waitcnt
+; GCN-NEXT: .Lfunc_end
+define i32 @test_tail_call_null() #0 {
+  %call = tail call i32 null(i32 1)
+  ret i32 %call
+}

test/CodeGen/AMDGPU/unsupported-calls.ll

 }
 
 ; GCN: :0:0: in function test_call_from_shader i32 (): unsupported call from graphics shader of function defined_function
 ; R600: in function test_call{{.*}}: unsupported call to function defined_function
 define amdgpu_ps i32 @test_call_from_shader() {
   %call = call i32 @defined_function(i32 0)
   ret i32 %call
 }
+
+; FIXME: Bad error message
+; GCN: error: <unknown>:0:0: in function test_call_absolute void (): unsupported indirect call to function <unknown>
+; R600: error: <unknown>:0:0: in function test_call_absolute void (): unsupported call to function <unknown>
+define amdgpu_kernel void @test_call_absolute() #0 {
+  %val = call i32 inttoptr (i64 1234 to i32(i32)*) (i32 1)
+  %op = add i32 %val, 1
+  store volatile i32 %op, i32 addrspace(1)* undef
+  ret void
+}