- User Since
- Oct 2 2017, 11:16 AM (107 w, 1 d)
Jan 7 2019
Jan 4 2019
Jan 3 2019
@hfinkel - I think this change is ready to merge. I've rebased it on current LLVM trunk and run the tests. I've also verified that the new test fails (with an assertion failure) on LLVM trunk without this change. Thanks!
Jan 2 2019
Rebasing on current LLVM master
Now that we have D38662, I'll look at getting this patch up to date and tested.
This patch has been replaced by D38662
Nov 7 2018
Nov 2 2018
I think this patch is updated and ready to go. I'v additionally confirmed that this patch resolves the issue (present on LLVM master) with the test in D38501.
Nov 1 2018
Fixing compilation problem. Now passes check-llvm.
Updating to LLVM master, fix comment, fix a check, add a test
Oct 31 2018
Note that this commit is related:
Nov 1 2017
This change is still important to me. What needs to happen next for it to make progress? Thanks!
Oct 12 2017
FWIW I've also reported this as this bug: https://bugs.llvm.org/show_bug.cgi?id=34937
Hi @mkazantsev , this change seems to have caused a regression. Before this change, the following program compiled with opt -gvn minimal64.ll -S but after, it causes an assertion failure in the compiler. I've verified that this failure is still present on trunk as of r315579.
First, it appears that this changeset addresses the problem I was having. I'm running in to a new problem with 128-bit pointers when using trunk (vs LLVM 4 or 5) which I'll suggest a fix for / bug report once I've got something minimized. I'll be working on that as well as generating a better test case for this BasicAA issue.
Oct 11 2017
@hfinkel - I'm obviously not your main reviewer, but I did spend a few minutes looking at this.
Oct 10 2017
Since I don't have commit access, could somebody commit this for me please? Thanks.
Oct 6 2017
@hfinkel - I've made the change you requested. I think this one is good to go, but as I mentioned we need to commit it after another fix to BasicAA (such as D38499), or else the new test will fail with BasicAA assertions.
- Break from loop instead of asserting if size > 64 bits
Oct 5 2017
@aprantl / @hfinkel - I improved the test, does it look better? Oddly enough the code necessary to trigger the assert is in a section that isn't meaningfully changed by GVN. I just beefed up the test to also show a related case that GVN did usefully change. I'm having trouble doing any better than that with a reasonable amount of effort.
- [ValueTracking] Fix a misuse of APInt in GetPointerBaseWithConstantOffset
- Improve test
- Update comment for "complete object type"
Oct 4 2017
In my use case, I have 128-bit pointers but "adding" to one of these will only ever modify the lower 64 bits. So for my use case, offsets > 64 bits just don't matter.
Hang on, there's a more fundamental problem here this is papering over. If your pointers are larger than 64 bits, those pointers can have offsets larger than 64 bits. Since BasicAA is using 64-bit integers to represent pointer offsets, the math in DecomposeGEPExpression will overflow, so you'll get incorrect results, and eventually cause a miscompile.
Thanks for the feedback!
- Improve comments, rename to getLLVMFieldNumber
Oct 3 2017
- Add a test of getFieldNumber (and others)
Oct 2 2017
This in some ways is a philosophical continuation of https://reviews.llvm.org/D35180 .