Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not do replaceFunction on a pointer return type #27

Merged
merged 2 commits into from
Nov 9, 2019
Merged

Conversation

wsmoses
Copy link
Member

@wsmoses wsmoses commented Nov 9, 2019

Calling convention for combined forward/reverse where the pointer is used isn't right -- this disables it (and some minor progress towards fixing but the larger fix is an optimization that will come in a bit).

@@ -2346,19 +2404,23 @@ Function* CreatePrimalAndGradient(Function* todiff, const std::set<unsigned>& co
}
}

// TODO IF OP IS POINTER
if (nonmarkedglobals_inactiveloads) {
Copy link
Contributor

@timkaler timkaler Nov 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having something like this (perhaps overly aggressive) is probably good for now. But, it might not be aggressive enough. For example, this doesn't seem to resolve the loading of @_ZGVZN5Eigen8internal20manage_caching_sizesENS_6ActionEPlS2_S2_E12m_cacheSizes

Corresponding to this function

`
/** \internal /
inline void manage_caching_sizes(Action action, std::ptrdiff_t
l1, std::ptrdiff_t* l2, std::ptrdiff_t* l3)
{
static CacheSizes m_cacheSizes;

if(action==SetAction)
{
// set the cpu cache size and cache all block sizes from a global cache size in byte
eigen_internal_assert(l1!=0 && l2!=0);
m_cacheSizes.m_l1 = *l1;
m_cacheSizes.m_l2 = *l2;
m_cacheSizes.m_l3 = *l3;
}
else if(action==GetAction)
{
eigen_internal_assert(l1!=0 && l2!=0);
*l1 = m_cacheSizes.m_l1;
*l2 = m_cacheSizes.m_l2;
*l3 = m_cacheSizes.m_l3;
}
else
{
eigen_internal_assert(false);
}
}
`
in http://pklab.med.harvard.edu/peterk/conos/scripts/cluster/scripts/seurat3/RcppEigen/include/Eigen/src/Core/products/GeneralBlockPanelKernel.h

It fails with error @_ZGVZN5Eigen8internal20manage_caching_sizesENS_6ActionEPlS2_S2_E12m_cacheSizes = linkonce_odr dso_local global i64 0, comdat, align 8
LLVM ERROR: cannot compute with global variable that doesn't have marked shadow global
make: *** [build/eigen_test-enzyme2] Error 1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you send me the full source for the test case you run with that and I'll take a look.

In any case this PR is not meant to resolve global issues (rather to resolve the returned pointer per #25), and should go in as is. I'll try the global code once you send me the file -- we should chat in person about globals on Monday since the conversation is likely more easier done in person rather than on a forum.

@timkaler
Copy link
Contributor

timkaler commented Nov 9, 2019

Looking at this now. First high level question is: are the mitigations you added for global variables part of this feature (i.e. did an issue arise as you were handling pointer return types that required some handling of global variables, or did you just include some support for global variables in this pull request as an unrelated feature). Main reason I ask is to determine whether I should think-about/test the degree to which this change fixes issues with globals we enounter in application benchmarks.

@wsmoses
Copy link
Member Author

wsmoses commented Nov 9, 2019

lol we have race conditions in commenting. No, the global variable stuff is not meant to resolve all global issues (but does a very minute one) and is not necessary for pointer returns (which is the true purpose of this branch). The globals bit was an easy fix I already had in my code so I left it in for now since I figure you might get use out of it.

@timkaler
Copy link
Contributor

timkaler commented Nov 9, 2019

resolves

lol we have race conditions in comenting. No global variable stuff is not meant to resolve all issues (but does a very minute one) -- this branch resolves the pointer return errors.

Got it. Looks good to me then. Resolves the pointer return type tests.

Copy link
Contributor

@timkaler timkaler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants