Page MenuHomePhabricator

[IRGen] Evaluate constant static variables referenced through member expressions
ClosedPublic

Authored by arphaman on Aug 18 2017, 7:53 AM.

Details

Summary

C++ allows us to reference static variables through member expressions. Prior to this patch, non-integer static variables that were referenced using a member expression were always emitted using lvalue loads. The old behaviour introduced an inconsistency between regular uses of static variables and member expressions uses, for example, the following program compiled and linked successfully:

struct Foo {
   constexpr static const char *name = "foo";
};
int main() {
  return Foo::name[0] == 'f';
}

but this program failed to link because "Foo::name" wasn't found:

struct Foo {
   constexpr static const char *name = "foo";
};
int main() {
  Foo f;
  return f.name[0] == 'f';
}

This patch ensures that constant static variables referenced through member expressions are emitted in the same way as ordinary static variable references.

rdar://33942261

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Aug 18 2017, 7:53 AM
arphaman edited the summary of this revision. (Show Details)
rsmith added inline comments.Aug 18 2017, 10:44 AM
lib/CodeGen/CGExprComplex.cpp
156 ↗(On Diff #111673)

If we can (correctly) allow side-effects in the initializer here, why can we not also do so when emitting a DeclRefExpr?

lib/CodeGen/CGExprScalar.cpp
1314–1321 ↗(On Diff #111673)

Can we remove this and just use tryEmit... unconditionally?

arphaman updated this revision to Diff 111956.Aug 21 2017, 5:16 AM
arphaman marked an inline comment as done.
  • Get rid of the AllowSideEffects argument.
  • Get rid of the old integer field evaluation code.
lib/CodeGen/CGExprComplex.cpp
156 ↗(On Diff #111673)

I think that DREs can't have side-effects in any case, so I think I can remove that argument.

rjmccall added inline comments.Aug 21 2017, 11:06 PM
lib/CodeGen/CGExpr.cpp
1323 ↗(On Diff #111956)

The side effects here are those associated with the initializer of the referenced declaration, not the DRE itself. Some expressions can be constant-evaluated despite having side-effects because the side-effects occur in an ignored operand, like the LHS of a comma or the base of a MemberExpr that refers to a static member.

We can't allow side effects here because (1) we're not actually collecting the side-effectful expressions to emit and (2) we'd need some permission from the context to decide that we're allowed to do so anyway (with a lambda capture, those side-effects have actually already been emitted, but I'm not convinced that's always true).

On the other hand, I think we need to be able to emit MemberExprs to static members as constants despite the presence of side-effects in their base expressions, and I can't think of any reasonable way to do that except actually making a temporary DeclRefExpr to try to constant-emit instead.

arphaman added inline comments.Aug 22 2017, 2:48 AM
lib/CodeGen/CGExpr.cpp
1323 ↗(On Diff #111956)

Thanks, I think I understand your explanation.

I will try using temporary a DRE instead when a static variable is used in a member expression. That means that the old integer field evaluation code will have to stay though.

arphaman updated this revision to Diff 112149.Aug 22 2017, 4:35 AM

Revert most of the changes and use temporary DREs as suggested by John.

rjmccall added inline comments.Aug 22 2017, 8:54 PM
lib/CodeGen/CGExprComplex.cpp
163 ↗(On Diff #112149)

There's an EmitIgnoredExpr you could use.

Also, I think it would be fine to add a generic tryEmitMemberExprAsConstant that takes a MemberExpr and does this DRE stuff behind the scenes; it's not at all different for the different emitters.

rjmccall added inline comments.Aug 22 2017, 8:58 PM
lib/CodeGen/CGExprComplex.cpp
163 ↗(On Diff #112149)

Well, actually, now I see why it's different for the complex emitter, but I think you could probably extract out a function that forms a complex pair from a constant output without too much trouble.

Also, as long as you're working with this, I think it's likely that the Agg emitter needs to handle this, too. I'm not sure I accept the claim there that constant r-value emission never applies to aggregates, but at the very least you need to handle references just as the DRE case does.

arphaman updated this revision to Diff 112360.Aug 23 2017, 7:01 AM
arphaman marked 2 inline comments as done.
  • Create tryEmitAsConstant for MemberExprs and emitConstant helper for complex values as suggested by John.
  • Remove dead constant emission code from CGExprAggregate.cpp
  • Emit static variable member expressions as DREs when emitting Lvalues to ensure the IR is consistent between DREs and MEs that reference a constant static variables whose type is a reference to an aggregate
lib/CodeGen/CGExprComplex.cpp
163 ↗(On Diff #112149)

It looks like that constant reference code in Agg emitter is dead, so I removed it.

The static constant variables that were references to aggregates were still inconsistent between DREs and MEs, so I now try to emit MEs as DREs in CodeGenFunction's Lvalue emitter now.

rjmccall accepted this revision.Aug 23 2017, 10:08 AM

Looks great, thanks.

This revision is now accepted and ready to land.Aug 23 2017, 10:08 AM
This revision was automatically updated to reflect the committed changes.