nemanjai added a comment.
"Made a new phabricator review because of git issues" is not an appropriate
description of a review/revision.
Hopefully the description you add will describe what this intrinsic is supposed
to do. It seems to me that this is a poorly designed feature if it is meant to
work the way it was implemented. Namely, it seems like this intrinsic clobbers
the stack protect global value rather than clobbering the corresponding value
on the stack for the specific function it is enclosed in. I would have thought
that it will clobber the stack in the function, thereby allowing stack
protection to work as expected for other functions in the module.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5014
+
+ if (IntrinsicID == Intrinsic::ppc_kill_canary) {
+ CurDAG->SelectNodeTo(N, PPC::NOP, MVT::Other, N->getOperand(0));
----------------
I think it would be preferable to handle this intrinsic in one place. The `nop`
is not actually necessary here. We should simply remove the intrinsic from the
stream in `PPCISelLowering.cpp` and not pass it on.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11132
+ case Intrinsic::ppc_kill_canary: {
+ MachineFunction &MF = DAG.getMachineFunction();
+ if (MF.getFunction().hasFnAttribute(Attribute::SafeStack) ||
----------------
The formatting of this entire block is quite messed up. Please run
`clang-format` on this.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11138
+
+ IRBuilder<> B(&MF.getFunction().getEntryBlock().front());
+
----------------
Do we use this?
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11144
+
+ if (GV == nullptr) break;
+ EVT VT = DAG.getTargetLoweringInfo().getValueType(DAG.getDataLayout(),
GV->getType(), true);
----------------
Is it ok to just ignore a failure to get the GV here? Should this not be an
assert?
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11156-11168
+ SDValue Store = DAG.getStore(
+ Op->getOperand(0),
+ DL,
+ DAG.getNode(
+ ISD::XOR,
+ DL,
+ VT,
----------------
What is happening here? We load the value, XOR it with itself, store it again?
Isn't that just zeroing it out? Why do we even need to load it then?
================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
+; RUN: --ppc-asm-full-reg-names < %s | FileCheck %s
----------------
At the very least, this has to also include a RUN line for Linux.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128652/new/
https://reviews.llvm.org/D128652
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
