[PATCH] D61626: [RISCV] Disable tail call if the callee function contain __builtin_frame_address or __builtin_return_address

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 marked 3 inline comments as done.
shiva0217 added a comment.

In D61626#1493735 <https://reviews.llvm.org/D61626#1493735>, @hfinkel wrote:

> Is this really a target-independent problem?


Hi @hfinkel,
I think you're right, it should be a target-independent problem. I have created D61665 <https://reviews.llvm.org/D61665>, Thanks.



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1312
+// Return frame depth argument of __builtin_frame_address or
+// __builtin_return_address
+static unsigned getFrameDepthArg(const Instruction *I) {
----------------
mgrang wrote:
> Period at the end of comment.
Got it, thanks.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1415
+    const Function *CalleeFn = dyn_cast<Function>(G->getGlobal());
+    if (CalleeFn)
+      for (const BasicBlock &BB : *CalleeFn)
----------------
mgrang wrote:
> This check can be combined with the assignment:
> if (const Function *CalleeFn = dyn_cast<Function>(G->getGlobal()))
Got it, thanks.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1419
+          if (isFrameAddressOrReturnAddressCall(&I) &&
+              (getFrameDepthArg(&I) > 0))
+            return false;
----------------
mgrang wrote:
> getFrameDepthArg asserts on isFrameAddressOrReturnAddressCall. So we end up calling isFrameAddressOrReturnAddressCall twice here. We could put them into one function but it is cleaner the way it is now.
> @asb Do you think this should be cleaned up?
Hi @mgrang and @asb,
I created D61665 to replace this one and avoid calling isFrameAddressOrReturnAddressCall twice in the new patch.
Do you happy with the new implementation?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61626