Made a new phabricator review because of git issues
|50 ms||x64 debian > LLVM.CodeGen/PowerPC::kill-canary-intrinsic.ll|
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix --ppc-asm-full-reg-names < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll -check-prefix=AIX
"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.
|5014 ↗||(On Diff #440234)|
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.
The formatting of this entire block is quite messed up. Please run clang-format on this.
Do we use this?
Is it ok to just ignore a failure to get the GV here? Should this not be an assert?
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?
At the very least, this has to also include a RUN line for Linux.
Please make sure the patch is able to compile first. I fixed two build failures, it still fails : (
Need rebase your code. GCCBuiltin is not a valid class anymore. See https://reviews.llvm.org/D127460
|5013 ↗||(On Diff #441374)|
Nit: avoid this unnecessary change
Do we need this header? A little strange...
Please make sure when you post the patch, it is ok to compile. DL seems undefined, we use dl here.
Also please read the coding style of llvm first. https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
canaryLoc does not follow the style, should be CanaryLoc.
All these new added codes need clang-format
|5 ↗||(On Diff #441374)|
We can merge these two files kill-canary-intrinsic-aix.ll and kill-canary-intrinsic-linux.ll into one file with different runlines but with different check prefix.