- User Since
- Jan 30 2014, 6:27 AM (340 w, 1 d)
The assertion doesn't work alone. I would still prefer not to skip over non-call sites blindly. @fhahn in addition to blockaddr users, do we expect anything else? We can skip those just fine.
IIRC, not all globals are dereferenceable and we have to look at the linkage. Could be wrong though.
Also clang format the patch please.
This diff is only against the last version, squash the commits locally before merging them into master. LGTM, apologies for the long time this took.
This seems to be related to the push of the cpu-id (aka. target-id) into other places as well: e.g., D84822, D80750, probably more.
I mentioned in those reviews already, this doesn't strike me as an "AMDGPU" feature at all.
We should start the discussions around missing functionality instead of adding AMDGPU workarounds in all these places...
One example is an IR module level target cpu and/or target feature, similar to target triple.
This is now patch number 3 or so that want's to introduce duplication of existing functionality for AMDGPU. This is an alarming trend. @gregrodgers
Is this still happening, I might have fixed the IRBuilder.
LGTM. Nice and small :) Thx!
Wed, Aug 5
This was quick, we need to write an RFC now ;). For this we should probably write the lang ref part too.
We should also mention somewhere this is the first step to properly fix PR965 and make the llvm.sideeffect obsolete (for this use case).
What is "cl/324220676" and how to I see it?
wrong attribute file ;)
The commit does not have a message, contains unrelated parts, and is still not addressing my issues wrt. the description of common functionality as AMDGPU functionality.
[DriveBy] We should test this ;)
Tue, Aug 4
Maybe change the commit title to OpenMP to confuse less people.
Note: @aqjune, and @okura We should also seed noundef in the intrinsics and especially known library functions definitions. I mean, malloc is not returning a undef or poison value (I hope), and free is not accepting one.
This looks fine to me but it doesn't impact me at all TBH.
There is a typo in your subject line [Attributor].
Forgot to say, this is really cool :) I'll take a closer look in a bit.
Mon, Aug 3
I think this is good. Let's see if there are objections
LGTM. Some minor nits below. I guess this doesn't affect other tests because of the noundef restriction. We should really derive that one as well.
Sun, Aug 2
Will update the clang tests shortly.
Sat, Aug 1
Follow up with the interprocedural replacement sounds good.
Fri, Jul 31
This is still "intra-procedural", correct? Or can it replace across function boundaries too?
Thu, Jul 30
LGTM, one nit below
Don't worry about the other test, master is sometimes broken as well. I left one comment for a follow up patch and one comment for a cleanup to avoid code duplication we have now.
Assuming that can be done without problems prior to committing this, the patch looks good to me :)
I am not sure if we want to go that route, maybe we land this patch like this for now and if we have hard time interpreting this data, we can add that later ?
Really cool :) LGTM
FWIW, I don't try to block this. The call site attribute comment is not addressed but other than that this patch "makes sense" assuming we keep the current semantics.
Wed, Jul 29
As discussed on IRC, I think noalias is actually what you want. Storing a pointer and loading it does *not* break the "based on" relationship (https://llvm.org/docs/LangRef.html#pointeraliasing). We might want to clarify the rules though.
I looked at some and we need to check what is happening.
Tue, Jul 28
Can you, maybe as a separate patch, run the update script with the call site deduction enabled *before* this patch and *after* to get the difference on all tests we have?
Thanks for working on this! Reachability should be used more and with an actual optimistic implementation it makes sense. The next user (in addition to AANoAlias) could be value simplification, for load/store simplification. Similarly to the noalias case we can preserve other properties for a program point if it is not reachable from all potential violations (I am thinking a use of a dereferenceable argument that cannot be reached from a function call that is not nofree. This makes sense once we split dereferenceable_globally from dereferenceable.) Let's get this in first though ;)
LGTM, assuming this is NFC and gets us closer to the NPM default :)
I believe there are three things in this patch, but feel free to correct me: