lebedev.ri added inline comments.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1694
// handle things like function to ptr-to-function decay etc.
Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
Expr *E = CE->getSubExpr();
----------------
vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > lebedev.ri wrote:
> > > > vsk wrote:
> > > > > I think maintaining a stack of visited cast exprs in the emitter be
> > > > > cheaper/simpler than using ASTContext::getParents. You could push CE
> > > > > here and use a RAII helper to pop it. The 'isCastPartOfExplicitCast'
> > > > > check then simplifies to a quick stack traversal.
> > > > Hmm, two things come to mind:
> > > > 1. This pessimizes the (most popular) case when the sanitizer is
> > > > disabled.
> > > > 2. `ASTContext::getParents()` may return more than one parent. I'm not
> > > > sure if that matters here?
> > > > I'll take a look..
> > > As for (1), it's not necessary to push/pop the stack when this sanitizer
> > > is disabled. And for (2), IIUC the only interesting case is
> > > "explicit-cast <implicit-cast>+", and none of the implicit casts here
> > > have more than one parent.
> > > I think maintaining a stack of visited cast exprs in the emitter be
> > > cheaper/simpler than using ASTContext::getParents.
> >
> > {F6660377}
> >
> > So yeah, this could work.
> > Except sadly it breaks in subtle cases like https://godbolt.org/g/5V2czU
> > I have added those tests beforehand.
> >
> > Is `ASTContext::getParents()` really horribly slow so we want to
> > duplicate/maintain/track
> > the current AST stack ourselves?
> > If so, we will need to maintain the *entire* stack, not just `CastExpr''s...
> I think the scan in 'IsTopCastPartOfExplicitCast' can be fixed: while
> traversing backwards, you'd need to check that the previously-visited cast
> expr is the child of the current expr. That should address the false negative
> you pointed out in interference1. I don't yet see what the issue is with
> interference0.
>
> Could you explain why maintaining a stack of unfinished casts wouldn't work?
> I don't understand why you'd need the entire stack. My sense is that it's not
> required to match the "explicit-cast <implicit-cast>+" pattern, but I could
> easily be missing something here. As for why this might be worth looking
> into, I think scanning for an explicit cast is much easier to understand when
> working with a stack.
>
> + @klimek to comment on what to expect in terms of the overhead of
> ASTContext::getParents. Regardless of what approach we pick, it would help to
> see pre/post-patch compile times for a stage2 build of something like clang
> or llc.
> I think the scan in 'IsTopCastPartOfExplicitCast' can be fixed: while
> traversing backwards, you'd need to check that the previously-visited cast
> expr is the child of the current expr. That should address the false negative
> you pointed out in interference1.
Oh right. That seems to fix the issues.
> Could you explain why maintaining a stack of unfinished casts wouldn't work?
I didn't think it wouldn't work. I just missed the tidbit about checking
children.
Then it works.
Repository:
rC Clang
https://reviews.llvm.org/D48958
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
