This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Unify visiting variable declarations
ClosedPublic

Authored by tbaeder on Oct 26 2022, 10:46 PM.

Details

Summary
We often visit the same variable multiple times, e.g. once when checking
its initializer and later when compiling the function. Unify both of
those in visitVarDecl() and do the returning of the value in
visitDecl().

I was working on something different and noticed that we register the same variable declaration twice.

Clang calls EvaluateAsInitializer() for local (const, probably) declarations as well, so we shouldn't register those as global variables.

Diff Detail

Event Timeline

tbaeder created this revision.Oct 26 2022, 10:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2022, 10:46 PM
tbaeder requested review of this revision.Oct 26 2022, 10:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2022, 10:46 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 471731.Oct 29 2022, 2:12 AM
tbaeder edited the summary of this revision. (Show Details)
tbaeder retitled this revision from [clang][Interp] Ignore visitDecl() for local declarations to [clang][Interp] Unify visiting variable declarations.Oct 29 2022, 11:34 PM
shafik added inline comments.Oct 30 2022, 9:49 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
803

Nitpick, I find using VD a bit confusing since we don't know if it is a ValueDecl until after it is initialized.

1154

I wonder if the code in both branches might be better factored out into their own functions and perhaps move the DeclScope<Emitter> LocalScope(this, VD); out.

clang/lib/AST/Interp/ByteCodeExprGen.h
282

Why not hasGlobalStorage()?

Also what is the logic behind isConstexpr() check?

tbaeder updated this revision to Diff 471905.Oct 30 2022, 11:47 PM
tbaeder marked an inline comment as done.
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
803

Replaced this with the same version we use a few lines below anyway.

clang/lib/AST/Interp/ByteCodeExprGen.h
282

Didn't know isGlobalStorage() existed ;)

Constexpr local variables can be handled like global ones, can't they? That was the logic behind it, nothing else. We can save ourselves the hassle of local variables in that case.

tbaeder updated this revision to Diff 471906.Oct 30 2022, 11:55 PM
shafik added inline comments.Oct 31 2022, 9:06 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
282

I think I am missing a level of logic here. I don't think of constant expressions as needing storage nor do I think of them as global variables.

So can you take a step back and explain how this fits in the bigger picture?

tbaeder added inline comments.Nov 1 2022, 8:36 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
282

They don't necessarily need storage in the final executable but we create global/local variables for them for bookkeeping, e.g. we need to be able to take their address, track the value, etc.

shafik added inline comments.Nov 3 2022, 2:13 PM
clang/lib/AST/Interp/ByteCodeExprGen.h
282

Ok, will this work for recursive constexpr functions with local variables?

tbaeder added inline comments.Nov 3 2022, 9:43 PM
clang/lib/AST/Interp/ByteCodeExprGen.h
282

local constexpr variables still have to be initialized and are immutable, so that doesn't make a difference for recursive functions since none of the recursive calls can change the variable value. I did a small local test but since the variable can't be modified, it's not very interesting.

aaron.ballman added inline comments.Nov 30 2022, 9:41 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
803
822
1127–1130

and if we have no GlobalIndex?

Should this instead be:

if (auto GlobalIndex = P.getGlobal(VD); !GlobalIndex || !this->emitGetPtrGlobal(*GlobalIndex, VD))
  return false;
1129–1131

Similar question here as above -- what if It == Locals.end()?

1161

What if Init is nullptr?

1165

Same here?

clang/lib/AST/Interp/ByteCodeExprGen.h
96

I was a bit surprised to not see override here -- I wonder if we should have some visual separation between the overrides and the non-overrides given the similarity in naming?

282

I find the interface to be confusing. If I want to know "is this a global decl", I am not going to expect a local constexpr variable to go "oh yeah, I totally am!". Perhaps this should be renamed to something more about the semantics, like shouldBeGloballyIndexed() or something along those lines?

tbaeder marked 3 inline comments as done.Dec 1 2022, 2:46 AM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1127–1130

I mean, that shouldn't happen because the visitVarDecl call before would've returned false in that case.

1161

Global variables (or local constexpr ones) must have an initializer, no?

tbaeder updated this revision to Diff 479219.Dec 1 2022, 2:47 AM
aaron.ballman added inline comments.Dec 1 2022, 7:20 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1127–1130

The use of an if makes it seem like it could fail. Perhaps we should drop the if and use an assert?

1161

Hmm, I was thinking of:

struct S {};
constexpr S s;

but now that I think on it, that still has a non-null init expression, so I think it's safe for us to assert Init is nonnull.

clang/lib/AST/Interp/ByteCodeExprGen.h
262
tbaeder marked 4 inline comments as done.Dec 1 2022, 7:33 AM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1127–1130

What do you think about returning a tuple(valid, offset) from visitVarDecl? Although that might make the call in ByteCodeStmtGen.cpp a little awkward.

tbaeder updated this revision to Diff 479294.Dec 1 2022, 7:40 AM
aaron.ballman added inline comments.Dec 1 2022, 8:05 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1127–1130

Hmmmm, I don't think I'd like to see a tuple, but seeing an llvm::ErrorOr<> or a std::optional<> might be reasonable. The downside is that the visit* functions then won't have a uniform return type (can't do ErrorOr<void> that I'm aware of)

erichkeane added inline comments.Dec 1 2022, 8:14 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1127–1130

The way I've seen in the past is to have it be an error-type containing a nullptr_t (or some sort of empty/meaningless struct).

So something like:

ErrorOr<Void>

tbaeder updated this revision to Diff 479526.Dec 1 2022, 11:14 PM

Meh, just use an assert :|

This revision is now accepted and ready to land.Dec 2 2022, 7:38 AM
shafik accepted this revision.Dec 20 2022, 2:25 PM

LGTM

clang/lib/AST/Interp/ByteCodeExprGen.cpp
1125

I don't get the comment.

tbaeder added inline comments.Dec 21 2022, 1:48 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1125

P.getGlobal() must return a global index and not std::nullopt, since the visitVarDecl() call above did not return false. It will return false if something failed, but it didn't and so the variable must be allocated already.

This revision was landed with ongoing or failed builds.Jan 19 2023, 1:46 AM
This revision was automatically updated to reflect the committed changes.