[PATCH] D59666: AMDGPU: HIP compiler option -finstrument-functions flag

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:2793-2795
+  // argument to '__builtin_return_address' must be a constant integer
+  if (verifyReturnAddressArgumentIsConstant(Op, DAG))
+    return SDValue();
----------------
verifyReturnAddressArgumentIsConstant should probably be removed in a separate patch. The verifier now rejects the IR, and the DAG should never be allowed to produce an invalid one


================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:2799-2801
+  // Return the frame info object for the current function. This object contains
+  // information about objects allocated on the stack frame of the
+  // current function in an abstract way.
----------------
Unnecessary comment


================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:2807-2810
+  // This node represents llvm.returnaddress on DAG. Zero indicates the
+  // current function's return address
+  assert((cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue() == 0) &&
+      "Return address can be determined only for current frame.");
----------------
This should not be an assert. According to the spec, this could just return 0 for another frame id


================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:2815
+  // Mark it as an implicit live-in
+  unsigned Reg = MF.addLiveIn(AMDGPU::SGPR30_SGPR31, getRegClassFor(VT));
+
----------------
Should use TRI::getReturnAddressReg rather than hardcoding it in another place. You can just hardcode the register class though


================
Comment at: test/CodeGen/AMDGPU/returnaddress.ll:1
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck --check-prefix=GCN %s
+
----------------
Should use HSA triple


================
Comment at: test/CodeGen/AMDGPU/returnaddress.ll:9
+entry:
+  %0 = tail call i8* @llvm.returnaddress(i32 0)
+  ret i8* %0
----------------
Use named values


================
Comment at: test/CodeGen/AMDGPU/returnaddress.ll:12
+}
+
+declare i8* @llvm.returnaddress(i32) nounwind readnone
----------------
Needs test with non-0 frame


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59666/new/

https://reviews.llvm.org/D59666