DAG: Ignore call site attributes when emitting target intrinsic

[arsenm]

Summary

A target intrinsic may be defined as possibly reading memory,
but the call site may have additional knowledge that it doesn't read
memory. The intrinsic lowering will expect the pessimistic
assumption of the intrinsic definition, so the chain should
still be used.

[efriedma]

Why can't you just compute whether the intrinsic has a chain based on the IntrinsicInfo? Checking for a readnone attribute in the IR seems dubious at best; an intrinsic can be both readnone and require a chain (for example, it could have undefined behavior for some inputs).

[hfinkel]

In D26716#597575, @efriedma wrote:

Why can't you just compute whether the intrinsic has a chain based on the IntrinsicInfo? Checking for a readnone attribute in the IR seems dubious at best; an intrinsic can be both readnone and require a chain (for example, it could have undefined behavior for some inputs).

For what exactly are you asking? We can call getTgtMemIntrinsic, but the fall-back behavior would be the same (unless you want this to be a large behavioral change). The UB issue is potentially important, if we have readonly/readnone target intrinsics with UB, but that seems like a pre-existing issue.

[efriedma]

llvm.sqrt is a readnone intrinsic which can exhibit undefined behavior, but I think we don't actually generate a chain for it at the moment, so maybe we can get away with querying whether the intrinsic is readnone. But the right way to do that is to call Intrinsic::getAttributes; we can't trust the IR to have the right attributes on the declaration (it's not something the verifier checks).

[efriedma]

Err, actually that's wrong; we do ensure that intrinsics have the right attributes. So I guess this is okay as-is.

[arsenm]

In D26716#597575, @efriedma wrote:

Why can't you just compute whether the intrinsic has a chain based on the IntrinsicInfo? Checking for a readnone attribute in the IR seems dubious at best; an intrinsic can be both readnone and require a chain (for example, it could have undefined behavior for some inputs).

In D26716#601558, @efriedma wrote:

llvm.sqrt is a readnone intrinsic which can exhibit undefined behavior, but I think we don't actually generate a chain for it at the moment, so maybe we can get away with querying whether the intrinsic is readnone. But the right way to do that is to call Intrinsic::getAttributes; we can't trust the IR to have the right attributes on the declaration (it's not something the verifier checks).

Intrinsic declarations do have the right attributes on the declaration. The Function constructor ensures they are added.

[arsenm]

In D26716#601584, @arsenm wrote:

In D26716#597575, @efriedma wrote:

Why can't you just compute whether the intrinsic has a chain based on the IntrinsicInfo? Checking for a readnone attribute in the IR seems dubious at best; an intrinsic can be both readnone and require a chain (for example, it could have undefined behavior for some inputs).

In D26716#601558, @efriedma wrote:

llvm.sqrt is a readnone intrinsic which can exhibit undefined behavior, but I think we don't actually generate a chain for it at the moment, so maybe we can get away with querying whether the intrinsic is readnone. But the right way to do that is to call Intrinsic::getAttributes; we can't trust the IR to have the right attributes on the declaration (it's not something the verifier checks).

Intrinsic declarations do have the right attributes on the declaration. The Function constructor ensures they are added.

As a special case,you can have intrinsics defined in the backend which are just generally broken right now. The TargetIntrinsicInfo interface does exist to fix this, but needs work to make actually usable (and probably not worth the effort).

[arsenm]

r287593

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitAtomicStore(const StoreInst &I) {
 
   DAG.setRoot(OutChain);
 }
 
 /// visitTargetIntrinsic - Lower a call of a target intrinsic to an INTRINSIC
 /// node.
 void SelectionDAGBuilder::visitTargetIntrinsic(const CallInst &I,
                                                unsigned Intrinsic) {
-  bool HasChain = !I.doesNotAccessMemory();
-  bool OnlyLoad = HasChain && I.onlyReadsMemory();
+  // Ignore the callsite's attributes. A specific call site may be marked with
+  // readnone, but the lowering code will expect the chain based on the
+  // definition.
+  const Function *F = I.getCalledFunction();
+  bool HasChain = !F->doesNotAccessMemory();
+  bool OnlyLoad = HasChain && F->onlyReadsMemory();
 
   // Build the operand list.
   SmallVector<SDValue, 8> Ops;
   if (HasChain) {  // If this intrinsic has side-effects, chainify it.
     if (OnlyLoad) {
       // We don't need to serialize loads against other loads.
       Ops.push_back(DAG.getRoot());
     } else {

test/CodeGen/AMDGPU/llvm.amdgcn.s.getreg.ll

-; RUN: llc -march=amdgcn -mcpu=tahiti -verify-machineinstrs < %s | FileCheck %s
-; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=kaveri -verify-machineinstrs < %s | FileCheck %s
-; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=fiji -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -march=amdgcn -mcpu=tahiti -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=fiji -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
-; FUNC-LABEL: {{^}}s_getreg_test:
-; CHECK: s_getreg_b32 s{{[0-9]+}}, hwreg(HW_REG_LDS_ALLOC, 8, 23)
+; GCN-LABEL: {{^}}s_getreg_test:
+; GCN: s_getreg_b32 s{{[0-9]+}}, hwreg(HW_REG_LDS_ALLOC, 8, 23)
 define void @s_getreg_test(i32 addrspace(1)* %out) { ; simm16=45574 for lds size.
-  %lds_size_64dwords = call i32 @llvm.amdgcn.s.getreg(i32 45574) #0
+  %lds_size_64dwords = call i32 @llvm.amdgcn.s.getreg(i32 45574)
+  %lds_size_bytes = shl i32 %lds_size_64dwords, 8
+  store i32 %lds_size_bytes, i32 addrspace(1)* %out
+  ret void
+}
+
+; Call site has additional readnone knowledge.
+; GCN-LABEL: {{^}}readnone_s_getreg_test:
+; GCN: s_getreg_b32 s{{[0-9]+}}, hwreg(HW_REG_LDS_ALLOC, 8, 23)
+define void @readnone_s_getreg_test(i32 addrspace(1)* %out) { ; simm16=45574 for lds size.
+  %lds_size_64dwords = call i32 @llvm.amdgcn.s.getreg(i32 45574) #1
   %lds_size_bytes = shl i32 %lds_size_64dwords, 8
   store i32 %lds_size_bytes, i32 addrspace(1)* %out
   ret void
 }
 
 declare i32 @llvm.amdgcn.s.getreg(i32) #0
 
-attributes #0 = { nounwind readonly}
+attributes #0 = { nounwind readonly }
+attributes #1 = { nounwind readnone }