[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

jdoerfert added a comment. Please modify the commit subject and add a proper message. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 305603. Herald added a subscriber: lxfind. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 Files: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/CGCall.cpp clang/test/CXX/except/except.spec/p14-ir.cpp

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added inline comments. Comment at: clang/test/CodeGenCXX/this-nonnull.cpp:1-2 +// RUN: %clang_cc1 -S -emit-llvm -o - -triple %itanium_abi_triple %s | FileCheck %s -check-prefix=CHECK-YES +// RUN: %clang_cc1 -S -emit-llvm -o - -fno-delete-null-pointer-checks -triple

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 303648. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 Files: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/CGCall.cpp clang/test/CXX/except/except.spec/p14-ir.cpp

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/CodeGenCXX/this-nonnull.cpp:1-2 +// RUN: %clang_cc1 -S -emit-llvm -o - -triple %itanium_abi_triple %s | FileCheck %s -check-prefix=CHECK-YES +//

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson marked an inline comment as done. CJ-Johnson added a comment. In D17993#2367447 , @rsmith wrote: > Thanks! We should also have a test for the behavior when targeting the MS > ABI, where we sometimes don't emit the `nonnull dereferenceable`

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 302236. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 Files: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/CGCall.cpp clang/test/CXX/except/except.spec/p14-ir.cpp

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rsmith added a comment. Thanks! We should also have a test for the behavior when targeting the MS ABI, where we sometimes don't emit the `nonnull dereferenceable` because the "`this`" pointer might actually point outside the object, but otherwise I think this is ready to go. Please can you

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added a comment. The new tests can be found in this-nonnull.cpp: https://reviews.llvm.org/differential/changeset/?ref=2242268 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 ___

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 302107. CJ-Johnson marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 Files: clang/lib/CodeGen/CGCall.cpp clang/test/CXX/except/except.spec/p14-ir.cpp

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson marked an inline comment as done. CJ-Johnson added inline comments. Comment at: clang/test/CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp:126 // -// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* %[[RIGHT]]) +// CHECK: call x86_thiscallcc void

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2165 +assert(getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0 && + "Expected `this` pointer without address space attribute."); + jdoerfert wrote: > I'm unsure why

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2174 + } + unsigned ArgNo = 0; rsmith wrote: > CJ-Johnson wrote: > > jdoerfert wrote: > > > Even if null pointer is valid we should place dereferenceable. > > > > > > We also could

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. A lot of the test changes here seem to be over-constraining the existing tests. Tests that don't care about `nonnull` / `dereferenceable` `this` pointers should generally not be

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

jdoerfert added subscribers: aaron.ballman, lebedev.ri. jdoerfert added a comment. Overall this looks sane to me. Don't know who wants to accept this. @rjmccall @lebedev.ri @aaron.ballman @rsmith Comment at: clang/lib/CodeGen/CGCall.cpp:2165 +

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2174 + } + unsigned ArgNo = 0; jdoerfert wrote: > Even if null pointer is valid we should place dereferenceable. > > We also could never place nonnull and let the middle-end make the

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 297655. CJ-Johnson added a comment. Apply dereferenceable even if null is a valid address CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 Files: clang/lib/CodeGen/CGCall.cpp

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added inline comments. Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:1082 +bool isGFX9Plus(const MCSubtargetInfo ) { + return isGFX9(STI) || isGFX10(STI); +} CJ-Johnson wrote: > arsenm wrote: > > How are these changes related? > This is

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2174 + } + unsigned ArgNo = 0; Even if null pointer is valid we should place dereferenceable. We also could never place nonnull and let the middle-end make the dereferenceable ->

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

jdoerfert added a comment. I think this is the wrong patch now. @xbolva00 posted a command that works if `git show HEAD` shows the commit you want to upload. If not, replace HEAD with the commit hash. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added inline comments. Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:1082 +bool isGFX9Plus(const MCSubtargetInfo ) { + return isGFX9(STI) || isGFX10(STI); +} arsenm wrote: > How are these changes related? This is a mistake. I did not

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 297647. CJ-Johnson added a comment. Update with full diff context CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 Files: clang/lib/CodeGen/CGCall.cpp clang/test/CXX/except/except.spec/p14-ir.cpp

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:1082 +bool isGFX9Plus(const MCSubtargetInfo ) { + return isGFX9(STI) || isGFX10(STI); +} How are these changes related? CHANGES SINCE LAST ACTION

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 297645. CJ-Johnson added a comment. Herald added subscribers: llvm-commits, kerbowa, hiraditya, nhaehnle, arsenm. Herald added a project: LLVM. Update diff with full context CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

xbolva00 added a comment. In D17993#2325616 , @CJ-Johnson wrote: > In D17993#2325610 , @jdoerfert wrote: > >> Can you please upload again with full context? > > My apologies, I am new to LLVM contribution. What is

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added a comment. In D17993#2325610 , @jdoerfert wrote: > Can you please upload again with full context? My apologies, I am new to LLVM contribution. What is the best way to do that such that it squashes all of my local git commits? CHANGES

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

jdoerfert added a comment. Can you please upload again with full context? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson updated this revision to Diff 297640. CJ-Johnson edited the summary of this revision. CJ-Johnson added a comment. Herald added subscribers: jdoerfert, aheejin, sbc100, jvesely, dschuff. Rebasing on head, removing flag changes since that was added in https://reviews.llvm.org/D47894 and

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson commandeered this revision. CJ-Johnson added a reviewer: bkramer. CJ-Johnson added a comment. The patch is ready! Commandeering this change :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added a comment. After chatting with bkramer , I'm working on rebasing this diff so that it can be landed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

CJ-Johnson added inline comments. Comment at: include/clang/Driver/Options.td:1157 +def fno_delete_null_pointer_checks : Flag<["-"], "fno-delete-null-pointer-checks">, + Group, Flags<[CC1Option]>; rjmccall wrote: > Please include HelpText, and it's probably

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rjmccall added a comment. In the cases where we can emit nonnull, I agree that we should be able to safely use dereferenceable for the non-virtual data size of of the type. I'm not too concerned about people calling methods on entirely the wrong type, especially to the relatively small degree

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

gberry added a subscriber: gberry. gberry added a comment. Not to hijack this review, but would it not make sense to do something similar with the 'this' pointer and the 'dereferenceable' attribute? Granted, figuring out the size to use is trickier, but I believe this would enable LICM e.g. to

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

hfinkel added a comment. In http://reviews.llvm.org/D17993#371459, @chandlerc wrote: > In http://reviews.llvm.org/D17993#371454, @hfinkel wrote: > > > In http://reviews.llvm.org/D17993#370793, @chandlerc wrote: > > > > > If we're not going to fully implement "fdelete-null-pointer-checks" we > >

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

hfinkel added a comment. In http://reviews.llvm.org/D17993#370790, @rjmccall wrote: > Hal, I think you're talking about a slightly different thing. This patch is > adding an assumption that C++ this pointers are non-null, but only when > -fno-delete-null-pointer-checks is not passed. The

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

chandlerc added a comment. In http://reviews.llvm.org/D17993#371454, @hfinkel wrote: > In http://reviews.llvm.org/D17993#370793, @chandlerc wrote: > > > If we're not going to fully implement "fdelete-null-pointer-checks" we > > shouldn't claim to... I'm really worried about us accepting that

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

hfinkel added a comment. In http://reviews.llvm.org/D17993#370793, @chandlerc wrote: > If we're not going to fully implement "fdelete-null-pointer-checks" we > shouldn't claim to... I'm really worried about us accepting that flag and not > actually honoring it. > > However, I *do* think this

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rsmith added a comment. `-f(no-)strict-nonnull-this` maybe? http://reviews.llvm.org/D17993 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rjmccall added a comment. Well, no, we can do it under a different flag and just ensure that -fno-delete-null-pointer-checks *also* has the effect of disabling the optimization. But you are not inspiring to disagree with Chandler about whether this optimization should be enabled by default.

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rjmccall added a comment. Er, sorry. You are not inspiring *me* to disagree with Chandler. http://reviews.llvm.org/D17993 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

bkramer added a comment. GCC 6 is already doing this and people are already annotating their builds with -fno-delete-null-pointer-checks. Putting it under a different flag will break compatibility there :( http://reviews.llvm.org/D17993 ___

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

chandlerc added a subscriber: chandlerc. chandlerc added a comment. If we're not going to fully implement "fdelete-null-pointer-checks" we shouldn't claim to... I'm really worried about us accepting that flag and not actually honoring it. However, I *do* think this should be guarded by a flag,

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

rjmccall added a comment. Hal, I think you're talking about a slightly different thing. This patch is adding an assumption that C++ this pointers are non-null, but only when -fno-delete-null-pointer-checks is not passed. The flag is therefore at least somewhat functional. (I would argue

Re: [PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

hfinkel added a subscriber: hfinkel. hfinkel added a comment. I'm *really* nervous about doing anything with -f(no-)delete-null-pointer-checks that makes it look like we support this feature without actually supporting it in the backend. In computePointerICmp in InstructionSimplify.cpp, we

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

bkramer created this revision. bkramer added reviewers: rsmith, rjmccall. bkramer added a subscriber: cfe-commits. Also thread -f(no-)delete-null-pointer-checks through to CodeGen and make it disable this behavior. It's not a full implementation of that flag but it would be good to stay