Page MenuHomePhabricator

[CodeGen][typeid] Emit typeinfo directly if type is known at compile-time
ClosedPublic

Authored by zequanwu on Wed, Sep 9, 4:30 PM.

Diff Detail

Event Timeline

zequanwu created this revision.Wed, Sep 9, 4:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 9, 4:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zequanwu requested review of this revision.Wed, Sep 9, 4:30 PM
zequanwu updated this revision to Diff 290846.Wed, Sep 9, 4:33 PM

Remove unused field.

hans added a comment.Thu, Sep 10, 5:18 AM

I'm not sure that changing isPotentiallyEvaluated() is the right thing to do. The meaning of that corresponds to text in the standard: https://eel.is/c++draft/expr.typeid#3 so changing it to something that doesn't match the standard exactly seems wrong.

I think it would be safer to do the change purely as an optimization in codegen (maybe we could add a new helper method that could also be used by the warning).

Also, I found another interesting test case: https://godbolt.org/z/aW4ojr

#include <typeinfo>

class B {
public:
  virtual void f();
};

B f();

 const std::type_info& g() {
  return typeid(f());
}

Here MSVC warns about /GR- and emits the call to __RTtypeid even though I think it's not necessary since the type is statically known. Actually, if I understand things correctly, f() shouldn't even be evaluated here since the expression is not a glvalue, and Clang gets that right. So that's a case where our codegen is better than MSVC :-)

zequanwu updated this revision to Diff 291619.Mon, Sep 14, 11:00 AM

If the operand of CXXTypeidExpr is already most derived object, no need to look up vtable.

I'm not sure that changing isPotentiallyEvaluated() is the right thing to do. The meaning of that corresponds to text in the standard: https://eel.is/c++draft/expr.typeid#3 so changing it to something that doesn't match the standard exactly seems wrong.

If the type of the operand is already most derived, we could just emit the typeinfo of the type. This would match the standard: https://eel.is/c++draft/expr.typeid#2

I think it would be safer to do the change purely as an optimization in codegen (maybe we could add a new helper method that could also be used by the warning).

For "optimization in codegen", do you mean optimization after the IR is generated or like I did in CodeGenFunction::EmitCXXTypeidExpr?

hans added a comment.Tue, Sep 15, 10:04 AM

I think it would be safer to do the change purely as an optimization in codegen (maybe we could add a new helper method that could also be used by the warning).

For "optimization in codegen", do you mean optimization after the IR is generated or like I did in CodeGenFunction::EmitCXXTypeidExpr?

I mean like you did in CodeGenFunction::EmitCXXTypeidExpr.

clang/include/clang/AST/ExprCXX.h
861

A comment would be good here.

clang/lib/AST/ExprCXX.cpp
151

I suppose it depends on what you want the semantics of this function to be, but returning false here seems odd.. wouldn't true make as much sense?
If this function is not intended to be called for type operands, maybe assert about that instead?

154

I think checking for DeclRefExpr isn't guaranteed to handle all cases, like

typeid(*&obj)

for example. That's okay, but it should be clear from the function comment (and maybe the name) that this is a best-effort check, not a strong guarantee about whether the expression refers to a most derived object.

clang/test/SemaCXX/constant-expression-cxx2a.cpp
312 ↗(On Diff #291619)

This appears to be changing semantics. It could be that this test is unnecessary strict (that's my understanding), but usually these checks are based on examples in the standard, or the current understanding of the standard. I think it would be best to check with Richard before changing this.

Actually, I'm surprised this affected since you're only updating CodeGen, not Sema. Is the static_assert really invoking Codegen?

zequanwu updated this revision to Diff 291988.Tue, Sep 15, 11:29 AM

Address comment.

zequanwu marked 4 inline comments as done.Tue, Sep 15, 11:29 AM
zequanwu added inline comments.
clang/test/SemaCXX/constant-expression-cxx2a.cpp
312 ↗(On Diff #291619)

This was caused by my previous changes on isPotentiallyEvaluated, which is used by Sema. I reverted this.

hans accepted this revision.Tue, Sep 15, 12:04 PM

lgtm

clang/test/SemaCXX/constant-expression-cxx2a.cpp
312 ↗(On Diff #291619)

Ah, great. That makes more sense :)

This revision is now accepted and ready to land.Tue, Sep 15, 12:04 PM
This revision was landed with ongoing or failed builds.Tue, Sep 15, 12:17 PM
This revision was automatically updated to reflect the committed changes.
zequanwu marked an inline comment as done.