Add a special GC AS for array ptrs #28251
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Bugpointed test case for the LLVM bug: |
|
Introduced by the vectorizer: |
|
loop-vectorize only test case: |
|
Filed upstream as https://bugs.llvm.org/show_bug.cgi?id=38290 |
|
Candidate patch: |
|
Looks like there's another issue in loop-reduce. Bugpoint reduction: |
|
Loop reduce bug looks fixed on upstream master. I'll bisect to find the relevant commit |
|
The commit that fixes LSR is: |
|
Ok, everything builds and passes with those two patches. I'll go ahead and do those patches as a separate PR for ease of integration. |
vtjnash
approved these changes
Jul 26, 2018
| MDNode *tbaa = arraytype_constshape(tinfo.typ) ? tbaa_const : tbaa_arrayptr; | ||
| PointerType *PT = cast<PointerType>(addr->getType()); | ||
| PointerType *PPT = cast<PointerType>(PT->getElementType()); |
There was a problem hiding this comment.
I need the reference to PT anyway, so I think this is fine.
src/llvm-late-gc-lowering.cpp
Outdated
| if (auto PtrT = dyn_cast<PointerType>(LI->getType())) { | ||
| if (PtrT->getAddressSpace() == AddressSpace::Loaded) { | ||
| CurrentV = LI->getPointerOperand(); | ||
| if (getValueAddrSpace(CurrentV) == 0) { |
There was a problem hiding this comment.
should this be if (!isSpecialPtr(getValueAddrSpace(CurrentV))), or is the idea that we only want to allow the base pointer to be either Tracked or None
There was a problem hiding this comment.
We don't really have non-gc address spaces in a target that can do GC, so I don't think it matters too much, but I'll change it.
There was a problem hiding this comment.
I think they support having code in the gc address space, as long as it eventually (after optimizations) doesn't end up in the final code. I think only maleadt would know if this makes a difference here, so I'm probably just being overly conservative.
vtjnash
reviewed
Jul 26, 2018
doc/src/devdocs/llvm.md
Outdated
| @@ -211,6 +211,11 @@ three different address spaces (their numbers are defined in `src/codegen_shared | |||
| future), but unlike the other pointers need not be rooted if passed to a | |||
| call (they do still need to be rooted if they are live across another safepoint | |||
| between the definition and the call). | |||
| - Pointers loaded from tracked object (currently 13): This is used by arrays, | |||
| which themselves contain a data to the managed data. This data area is owned | |||
|
First patch is submitted upstream as https://reviews.llvm.org/D49832. |
|
Committed upstream as https://reviews.llvm.org/rL338072 / https://reviews.llvm.org/rL338073, but we can keep the D49832 version. It's functionally equivalent. |
The array data pointer is somewhat special. It points to a chunk for memory that is effectively managed by the GC, but is not itself a GC-tracked value. However, it is also not quite an interior pointer into the array, since it may be an external allocation (or at the more immediate IR level it is derived using a load rather than a gep). We could try to make Derived do both, but the semantics turn out to be rather different, so add a new kind of AS `Loaded`, that handles precisely this situation: It roots the object that it was loaded from while it is live. Fixes #27955
Keno
changed the title
KristofferC
pushed a commit
that referenced
this pull request
Feb 11, 2019
The array data pointer is somewhat special. It points to a chunk for memory that is effectively managed by the GC, but is not itself a GC-tracked value. However, it is also not quite an interior pointer into the array, since it may be an external allocation (or at the more immediate IR level it is derived using a load rather than a gep). We could try to make Derived do both, but the semantics turn out to be rather different, so add a new kind of AS `Loaded`, that handles precisely this situation: It roots the object that it was loaded from while it is live. Fixes #27955
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The array data pointer is somewhat special. It points to a chunk
for memory that is effectively managed by the GC, but is not itself
a GC-tracked value. However, it is also not quite an interior pointer
into the array, since it may be an external allocation (or at the
more immediate IR level it is derived using a load rather than
a gep). We could try to make Derived do both, but the semantics
turn out to be rather different, so add a new kind of AS
Loaded,that handles precisely this situation: It roots the object that it
was loaded from while it is live.
Fixes #27955
Current status: Seems to fix the issue, but seems to trigger LLVM bugs (optimizations introducing illegal ptrtoint).