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

Migrate to opaque pointers #553

Closed
elliottslaughter opened this issue May 19, 2022 · 6 comments
Closed

Migrate to opaque pointers #553

elliottslaughter opened this issue May 19, 2022 · 6 comments

Comments

@elliottslaughter
Copy link
Member

Recent LLVM versions have begun migrating to opaque pointers, i.e., pointers with no type. Notably, LLVM 14 added versions of CreateLoad etc. that require an explicit type argument (as the element type is no longer specified in the pointer type).

For now, non-opaque pointers aren't actually gone so we can rely on the old code paths. But presumably at some point LLVM will pull the plug and we'll need to migrate.

My initial effort to do the migration didn't go so well because there are a bunch of corner cases. Not all types are clearly labeled in the compiler, and sometimes they seem to be missing entirely (because we're relying on pointers to carry that information). So in order to work in a world with opaque pointers, we need to track all of that and make sure we don't lose it as it's going down the stack.

@elliottslaughter
Copy link
Member Author

This is now required as LLVM 15 has removed non-opaque pointers: https://releases.llvm.org/15.0.0/docs/ReleaseNotes.html#changes-to-the-llvm-ir

@rongduo

This comment was marked as spam.

@elliottslaughter
Copy link
Member Author

@rongduo you seem to be using an auto-responder on this issue tracker. Please stop. This is creating spam on the issues. I've seen this several times now and will need to block you if you don't disable the auto-responder.

@elliottslaughter
Copy link
Member Author

Actually, it looks like we're going to be forced to straddle this change. According to the official documentation, opaque pointers in LLVM 14 are not production ready. So we'll need to keep the non-opaque code paths until we support LLVM 15+ only.

https://llvm.org/docs/OpaquePointers.html#version-support

@elliottslaughter
Copy link
Member Author

The first part of this migration went into #639. I think at this point, there is only one nontrivial place in the code where we need pointer types:

terra/src/tcompiler.cpp

Lines 601 to 603 in 8f43bed

StructType *st = cast<StructType>(df->getFunctionType()
->getParamType(argpos)
->getPointerElementType());

This occurs in the context of trying to get external types out of Clang. Basically, we define a function that includes all of these types as arguments, and then when we need one, we grab that function and fetch the Nth argument to get its type.

terra/src/tcompiler.cpp

Lines 598 to 600 in 8f43bed

Function *df = TT->external->getFunction(name);
assert(df);
int argpos = typ->number("llvm_argumentposition");

If you look for llvm_definingfunction you'll find it here:

terra/src/tcwrapper.cpp

Lines 147 to 155 in 8f43bed

if (!tt->boolean("llvm_definingfunction")) {
size_t argpos = RegisterRecordType(Context->getRecordType(rd));
lua_pushstring(L, livenessfunction.c_str());
tt->setfield("llvm_definingfunction");
lua_pushinteger(L, TT->id);
tt->setfield("llvm_definingtarget");
lua_pushinteger(L, argpos);
tt->setfield("llvm_argumentposition");
}

We're basically building a list of types:

terra/src/tcwrapper.cpp

Lines 122 to 127 in 8f43bed

size_t RegisterRecordType(QualType T) {
outputtypes.push_back(Context->getPointerType(T));
assert(outputtypes.size() < 65536 &&
"fixme: clang limits number of arguments to 65536");
return outputtypes.size() - 1;
}

And we build a function with all the types as parameters:

terra/src/tcwrapper.cpp

Lines 501 to 524 in 8f43bed

FunctionDecl *GetLivenessFunction() {
IdentifierInfo &II = Context->Idents.get(livenessfunction);
DeclarationName N = Context->DeclarationNames.getIdentifier(&II);
QualType T = Context->getFunctionType(Context->VoidTy, outputtypes,
FunctionProtoType::ExtProtoInfo());
FunctionDecl *F = FunctionDecl::Create(
*Context, Context->getTranslationUnitDecl(), SourceLocation(),
SourceLocation(), N, T, 0, SC_Extern);
std::vector<ParmVarDecl *> params;
for (size_t i = 0; i < outputtypes.size(); i++) {
params.push_back(ParmVarDecl::Create(*Context, F, SourceLocation(),
SourceLocation(), 0, outputtypes[i],
/*TInfo=*/0, SC_None, 0));
}
F->setParams(params);
CompoundStmt *stmts = CompoundStmt::Create(*Context, outputstmts,
#if LLVM_VERSION >= 150
FPOptionsOverride(),
#endif
SourceLocation(), SourceLocation());
F->setBody(stmts);
return F;
}

It's not totally clear to me whether we're hitting this line or not, but we may also be doing some magic to keep the function itself alive (or this might be for other functions):

terra/src/tcwrapper.cpp

Lines 483 to 484 in 8f43bed

KeepLive(f); // make sure this function is live in codegen by creating a dummy
// reference to it (void) is to suppress unused warnings

Anyway, it's not obvious to me how to replace this machinery. The types we're passing to the function are all T*, so under opaque pointers, we lose all type information. I tried passing just T, but (a) this crashes Clang (I'm not sure Clang is used to handling functions with so many non-pointer parameters), and (b) in the process of lowering to LLVM, Clang would mangle the types anyway (because parts of the C calling convention are implemented in Clang). Similarly I don't think passing any other type (T[], struct S { T t; }, etc.) would work.

I think there are two things that a solution needs to accomplish:

  1. Make sure all imported types stay live so we can refer to them
  2. Make a way of passing them through to Terra so we can refer to the same types in our end

However, since I had to disable this code path in the past for other targets, I should also try disabling this conditional and see if we can get away with just not reusing the types at all:

terra/src/tcompiler.cpp

Lines 595 to 597 in 8f43bed

// Important: only use the externally defined type if is was
// defined in the current target
if (TT == CU->TT) {

I.e., treating every target as a non-native target.

@elliottslaughter
Copy link
Member Author

We finished this in #643.

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

No branches or pull requests

2 participants