Do inlining and inbounds more carefully #917
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
charleskawczynski
force-pushed
the
ck/careful_inline_inbounds
branch
from
a91edc3 to
fee971b
Compare
|
bors try |
tryBuild failed: |
charleskawczynski
force-pushed
the
ck/careful_inline_inbounds
branch
2 times, most recently
from
d2f3222 to
fc8ea88
Compare
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
4 tasks
charleskawczynski
force-pushed
the
ck/careful_inline_inbounds
branch
from
8e1ae7f to
6d9a5b5
Compare
|
bors try |
tryBuild failed: |
charleskawczynski
force-pushed
the
ck/careful_inline_inbounds
branch
from
6d9a5b5 to
21d9bfc
Compare
|
Alright, I think this should be fine so long we have a job that applies |
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Aug 26, 2022
917: Do inlining and inbounds more carefully r=charleskawczynski a=charleskawczynski This PR is an attempt to partially revert (and complete) #829, by: - Exchanging the use of ``@inline`` and ``@inbounds`` - Removing ``@inline`` for "expensive" functions that do not have or uses indices which require boundscheck elision - Applying inlining and ``@inbounds`` to slab functions The only thing that I'm not 100% sure about is the ``@inbounds`` around the threading loops, does what I've done look okay? cc `@simonbyrne` Supersedes #891. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
|
Build failed: |
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Aug 26, 2022
917: Do inlining and inbounds more carefully r=charleskawczynski a=charleskawczynski This PR is an attempt to partially revert (and extend/complete) #829, by: - More properly using ``@inline`,` ``@inbounds`` and `Base.`@propagate_inbounds`` - Removing ``@inline`` for "expensive" functions that do not have or uses indices which require boundscheck elision - Applying inlining and ``@inbounds`` to slab functions The only thing that I'm not 100% sure about is the ``@inbounds`` around the threading loops, does what I've done look okay? cc `@simonbyrne` Supersedes #891. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
|
Maybe squash first? Or we don't care? |
|
Ah, I probably should have, CI is almost finished, so I'm fine with letting it go. If the implicit stencil tests hang again then I'll make sure to squash after fixing that |
|
Build failed: |
|
Ok, canceled this, I don't want to completely tank CI for the apply operators--something really bad is going on with these tests (opt was going from 6 min to more than 50 min). I'll revert the compose ops, too, and then squash. Opened #923. |
charleskawczynski
force-pushed
the
ck/careful_inline_inbounds
branch
from
21d9bfc to
bc51fdd
Compare
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Aug 26, 2022
917: Do inlining and inbounds more carefully r=charleskawczynski a=charleskawczynski This PR is an attempt to partially revert (and extend/complete) #829, by: - More properly using ``@inline`,` ``@inbounds`` and `Base.`@propagate_inbounds`` - Removing ``@inline`` for "expensive" functions that do not have or uses indices which require boundscheck elision - Applying inlining and ``@inbounds`` to slab functions The only thing that I'm not 100% sure about is the ``@inbounds`` around the threading loops, does what I've done look okay? cc `@simonbyrne` Supersedes #891. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
|
Build failed:
|
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Aug 26, 2022
917: Do inlining and inbounds more carefully r=charleskawczynski a=charleskawczynski This PR is an attempt to partially revert (and extend/complete) #829, by: - More properly using ``@inline`,` ``@inbounds`` and `Base.`@propagate_inbounds`` - Removing ``@inline`` for "expensive" functions that do not have or uses indices which require boundscheck elision - Applying inlining and ``@inbounds`` to slab functions The only thing that I'm not 100% sure about is the ``@inbounds`` around the threading loops, does what I've done look okay? cc `@simonbyrne` Supersedes #891. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
|
Build failed: |
Delete unused function More inbounds and propagate inline More inlining adjustments More inline and inbounds adjustments Apply formatter Fix merge conflicts Add an inbounds back in Add more inbounds More inbounds and inlining More inbounds More inbounds Increase mem for implicit stencil test Try 30 GB mem request for implicit stencil test Undo inlining for compose stencils Revert alloc request and pointwise apply
charleskawczynski
force-pushed
the
ck/careful_inline_inbounds
branch
from
bc51fdd to
c0b7991
Compare
|
bors r+ |
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.
This PR is an attempt to partially revert (and extend/complete) #829, by:
@inline,@inboundsandBase.@propagate_inbounds@inlinefor "expensive" functions that do not have or uses indices which require boundscheck elision@inboundsto slab functionsThe only thing that I'm not 100% sure about is the
@inboundsaround the threading loops, does what I've done look okay? cc @simonbyrneSupersedes #891.