- User Since
- Jan 30 2014, 6:27 AM (281 w, 5 d)
I think this looks fine with one request for change: We "decided" to use enum attributes not string attributes (for willreturn and others).
Just realized that the basic attribute test in test/Bitcode/attributes.ll is missing, see https://reviews.llvm.org/D49165#change-K8gwLFRSXwEe for an example.
@reames Was the last comment a "LGTM"? I need to start getting these patches of my plate :(
Sorry for my delayed review, I want to move this ahead now and commit it.
Fri, Jun 21
Wed, Jun 19
Tue, Jun 18
Could you run this with the LLVM test suite ?
Do you see a difference in statistics, compile time and/or runtime?
Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?
Fri, Jun 14
Update all uses of isDereferenceablePointer
Update according to API suggestion
What is supposed to happen if the elements in the pointer vector have different underlying objects?
We need to document that in the comment and test it.
Thu, Jun 13
Thanks for the quick feedback!
Simplify the interface
I want to investigate the racy accesses further and make sure it is not a miscompile inside LLVM.
I extracted the test case (see below) but I was not seeing the ERROR. How did you run the test case to see a different value for Count?
Can you make the test use an enum version, thus nofree not "nofree".
Wed, Jun 12
DereferenceableOrNullBytesGlobally -> DereferenceableOrNullGloballyBytes
I give up on this for now. If there is interest in this we can revive it later.
Addressed comments: Getter/Setter rename + for loop
The getter/setter functions are called "DereferenceableBytesGlobally" not "DereferenceableGloballyBytes". I would like to get some feedback on this.
Add all the mechanics for dereferenceable_globally we have for dereferenceable
Add argument definitions
Remove the alignment part
We need more test cases, e.g. as the one below which shows some limits of the current impl.
Tue, Jun 11
@nicholas If you want to discuss any of this further, please say so.
Is this still an RFC or by now an accumulation of changes we actually want to make? I added to comments assuming the latter.
This looks good to me (LGTM). Give it a bit of time though for people familiar with this code to take a look if they want.
More comments. Can you diff it against the master branch so one can easily see all changes?
Generally fine, three smaller comments though.
This is a clarrification for some older comments.
Mon, Jun 10
Why should the return values in nonnull.ll be noalias?
LGTM as well. Thanks!
Use worklist instead of recursion, add tests
Sun, Jun 9
Why does this depend on D63046?
Move the isKnownWillReturn isAssumedWillReturn and the to string function to the base class. Then place the base class in the header. Otherwise, LGTM.
@pcc So I thought about this. I don't think printing all attributes always helps. Testing for a single "Function Attrs" line will be very brittle, the use of multiple check lines will make tests harder to read/write/maintain and the IR output is cluttered. What if we have an output like this one under a flag? It would have all the benefits this patch has and, as you seem to dislike it, it would be on an opt-in basis. Though, I'm still unsure under which circumstance this patch would be worse than the status quo.
Thanks for looking at this. I'll update the patch asap
Cleanup leftover arguments
Update to new Attributor design and more tests