GlobalISel: Handle llvm.read_register

[arsenm]

Summary

SelectionDAG has a bunch of machinery to defer this to selection time
for some reason. Just directly emit a copy during IRTranslator. The
x86 usage does somewhat questionably check hasFP, which could depend
on the whole function being at minimum translated.

This does lose the convergent bit if the callsite had it, which may be
a problem. We also lose that in general for intrinsics, which may also
be a problem.

[rengolin]

Added some context comments in D67601.

For this one: why only AMDGPU tests? Would be good to have at least ARM32/64, since they should be working with GlobalIsel, no?

Not sure what's the difference between buffer_store_dword and flat_store_dword to make a comment on that change.

[arsenm]

In D67602#1671346, @rengolin wrote:

Added some context comments in D67601.

For this one: why only AMDGPU tests? Would be good to have at least ARM32/64, since they should be working with GlobalIsel, no?

One target is sufficient for new feature additions

Not sure what's the difference between buffer_store_dword and flat_store_dword to make a comment on that change.

GlobalISel doesn't yet implement the buffer_store path for older targets, but it's irrelevant for the actual test

[rengolin]

In D67602#1684353, @arsenm wrote:

For this one: why only AMDGPU tests? Would be good to have at least ARM32/64, since they should be working with GlobalIsel, no?

One target is sufficient for new feature additions

Fair enough. LGTM, thanks!

PS: @rovka, do you know if we have similar tests on the Arm side? Would be good to have some, too.

[arsenm]

r373294

[paquette]

Even though this was reverted, I'm going to necromance it to point out an issue with the approach wrt AArch64 in the inline comments.

Maybe a G_READ_REGISTER/G_WRITE_REGISTER opcode would be a better approach?

lib/CodeGen/GlobalISel/IRTranslator.cpp

1529

TLI->getRegisterByName can't handle sysregs on the AArch64 side, since they're modelled as immediate operands. So, this approach can't work for AArch64.

(see llvm/test/CodeGen/AArch64/special-reg.ll for an example)

[rengolin]

lib/CodeGen/GlobalISel/IRTranslator.cpp

1529

By design, they can only get the stack pointer, I think on any target. It should be an assembler error otherwise.

[paquette]

lib/CodeGen/GlobalISel/IRTranslator.cpp

1529

I don't think that's true in AArch64. E.g.:

https://reviews.llvm.org/D53115

The test added there uses read/write_register to access other special registers. This change made the test added there fail.

There's also the special-reg.ll test I mentioned above which has stuff like this:

%reg = call i64 @llvm.read_register.i64(metadata !1)
...
!1 = !{!"daif"}

Which is supposed to produce this (which does not involve SP):

mrs x0, DAIF

So I don't think that the stack pointer is the only thing we have to think about here. (Unless I'm seriously misunderstanding something, which is entirely possible. :P)

[arsenm]

lib/CodeGen/GlobalISel/IRTranslator.cpp

1529

We use this for the EXEC mask (with the special consideration that the call site needs to be marked convergent)

[rengolin]

lib/CodeGen/GlobalISel/IRTranslator.cpp

1529

Is that exposed to the users? For example, by declaring a register variable asm("daif")?

Writing to such a "variable" would be potentially dangerous, no?

[arsenm]

lib/CodeGen/GlobalISel/IRTranslator.cpp

1529

Only through builtins and builtin library functions, not asm variables

[rengolin]

lib/CodeGen/GlobalISel/IRTranslator.cpp

1529

Right, sounds good, thanks!

lib/CodeGen/GlobalISel/IRTranslator.cpp

bool IRTranslator::translateKnownIntrinsic(const CallInst &CI, Intrinsic::ID ID,
   }
   case Intrinsic::invariant_end:
     return true;
   case Intrinsic::assume:
   case Intrinsic::var_annotation:
   case Intrinsic::sideeffect:
     // Discard annotate attributes, assumptions, and artificial side-effects.
     return true;
+  case Intrinsic::read_register: {
+    Value *Arg = CI.getArgOperand(0);
+    const Metadata *MD = cast<MetadataAsValue>(Arg)->getMetadata();
+    const MDString *RegStr = cast<MDString>(cast<MDNode>(MD)->getOperand(0));
+
+    auto *TLI = MF->getSubtarget().getTargetLowering();
+    Register Dst = getOrCreateVReg(CI);
+    EVT VT = TLI->getValueType(*DL, CI.getType());
+    Register Reg = TLI->getRegisterByName(RegStr->getString().data(), VT, *MF);
+    if (!Reg.isValid())
+      return false;
+
+    MIRBuilder.buildCopy(Dst, Reg);
+    return true;
+  }
   }
   return false;
 }
 
 bool IRTranslator::translateInlineAsm(const CallInst &CI,
                                       MachineIRBuilder &MIRBuilder) {
   const InlineAsm &IA = cast<InlineAsm>(*CI.getCalledValue());
   if (!IA.getConstraintString().empty())

test/CodeGen/AMDGPU/GlobalISel/read_register.ll

+; Runs original SDAG test with -global-isel
+; RUN: llc -global-isel -mtriple=amdgcn-amd-amdhsa -mcpu=bonaire -verify-machineinstrs < %S/../read_register.ll | FileCheck -enable-var-scope %S/../read_register.ll

test/CodeGen/AMDGPU/read_register.ll

-; RUN: llc -march=amdgcn -mcpu=bonaire -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=bonaire -verify-machineinstrs < %s | FileCheck -enable-var-scope %s
 
 declare i32 @llvm.read_register.i32(metadata) #0
 declare i64 @llvm.read_register.i64(metadata) #0
 
 ; FIXME: Should be able to eliminate copy
 ; CHECK-LABEL: {{^}}test_read_m0:
 ; CHECK: s_mov_b32 m0, -1
 ; CHECK: s_mov_b32 [[COPY_M0:s[0-9]+]], m0
 ; CHECK: v_mov_b32_e32 [[COPY:v[0-9]+]], [[COPY_M0]]
-; CHECK: buffer_store_dword [[COPY]]
+; CHECK: flat_store_dword v{{\[[0-9]+:[0-9]+\]}}, [[COPY]]
 define amdgpu_kernel void @test_read_m0(i32 addrspace(1)* %out) #0 {
   store volatile i32 0, i32 addrspace(3)* undef
   %m0 = call i32 @llvm.read_register.i32(metadata !0)
   store i32 %m0, i32 addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_read_exec:
 ; CHECK: v_mov_b32_e32 v[[LO:[0-9]+]], exec_lo
 ; CHECK: v_mov_b32_e32 v[[HI:[0-9]+]], exec_hi
-; CHECK: buffer_store_dwordx2 v{{\[}}[[LO]]:[[HI]]{{\]}}
+; CHECK: flat_store_dwordx2 v{{\[[0-9]+:[0-9]+\]}}, v{{\[}}[[LO]]:[[HI]]{{\]}}
 define amdgpu_kernel void @test_read_exec(i64 addrspace(1)* %out) #0 {
   %exec = call i64 @llvm.read_register.i64(metadata !1)
   store i64 %exec, i64 addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_read_flat_scratch:
 ; CHECK: v_mov_b32_e32 v[[LO:[0-9]+]], flat_scratch_lo
 ; CHECK: v_mov_b32_e32 v[[HI:[0-9]+]], flat_scratch_hi
-; CHECK: buffer_store_dwordx2 v{{\[}}[[LO]]:[[HI]]{{\]}}
+; CHECK: flat_store_dwordx2 v{{\[[0-9]+:[0-9]+\]}}, v{{\[}}[[LO]]:[[HI]]{{\]}}
 define amdgpu_kernel void @test_read_flat_scratch(i64 addrspace(1)* %out) #0 {
   %flat_scratch = call i64 @llvm.read_register.i64(metadata !2)
   store i64 %flat_scratch, i64 addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_read_flat_scratch_lo:
 ; CHECK: v_mov_b32_e32 [[COPY:v[0-9]+]], flat_scratch_lo
-; CHECK: buffer_store_dword [[COPY]]
+; CHECK: flat_store_dword v{{\[[0-9]+:[0-9]+\]}}, [[COPY]]
 define amdgpu_kernel void @test_read_flat_scratch_lo(i32 addrspace(1)* %out) #0 {
   %flat_scratch_lo = call i32 @llvm.read_register.i32(metadata !3)
   store i32 %flat_scratch_lo, i32 addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_read_flat_scratch_hi:
 ; CHECK: v_mov_b32_e32 [[COPY:v[0-9]+]], flat_scratch_hi
-; CHECK: buffer_store_dword [[COPY]]
+; CHECK: flat_store_dword v{{\[[0-9]+:[0-9]+\]}}, [[COPY]]
 define amdgpu_kernel void @test_read_flat_scratch_hi(i32 addrspace(1)* %out) #0 {
   %flat_scratch_hi = call i32 @llvm.read_register.i32(metadata !4)
   store i32 %flat_scratch_hi, i32 addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_read_exec_lo:
 ; CHECK: v_mov_b32_e32 [[COPY:v[0-9]+]], exec_lo
-; CHECK: buffer_store_dword [[COPY]]
+; CHECK: flat_store_dword v{{\[[0-9]+:[0-9]+\]}}, [[COPY]]
 define amdgpu_kernel void @test_read_exec_lo(i32 addrspace(1)* %out) #0 {
   %exec_lo = call i32 @llvm.read_register.i32(metadata !5)
   store i32 %exec_lo, i32 addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_read_exec_hi:
 ; CHECK: v_mov_b32_e32 [[COPY:v[0-9]+]], exec_hi
-; CHECK: buffer_store_dword [[COPY]]
+; CHECK: flat_store_dword v{{\[[0-9]+:[0-9]+\]}}, [[COPY]]
 define amdgpu_kernel void @test_read_exec_hi(i32 addrspace(1)* %out) #0 {
   %exec_hi = call i32 @llvm.read_register.i32(metadata !6)
   store i32 %exec_hi, i32 addrspace(1)* %out
   ret void
 }
 
 attributes #0 = { nounwind }
 
 !0 = !{!"m0"}
 !1 = !{!"exec"}
 !2 = !{!"flat_scratch"}
 !3 = !{!"flat_scratch_lo"}
 !4 = !{!"flat_scratch_hi"}
 !5 = !{!"exec_lo"}
 !6 = !{!"exec_hi"}