[PATCH] D118437: [NFC] [Modules] Refactor ODR checking for default template argument in ASTReader

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:10176-10184
+            if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(D))
+              return TTP->hasDefaultArgument() &&
+                      !TTP->defaultArgumentWasInherited();
+            if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(D))
+              return NTTP->hasDefaultArgument() &&
+                      !NTTP->defaultArgumentWasInherited();
+            auto *TTP = cast<TemplateTemplateParmDecl>(D);
----------------
urnathan wrote:
> Rather than use dyn_cast, can't you use the switch(getKind()) idiom of the original?  That'll reduce the number of virtual calls won't it?  Bonus points for passing getKind() into the lambda so the compiler has a chance of noticing a loop unswitching possibility?
`dyn_cast` is implemented as "isa<X>(Val) ? cast<X>(Val) : nullptr;". Maybe the judgement here is what you says 'virtual call', isn't it? So it looks like the switch version would be better. From the source code, the switch version contains one call to getKind() and at most 3 comparison with int. But the current version would contain 2 calls to `isa<>` and 3 calls to `cast<>` and 4 null check. So we might think the switch version is better.

But on the one hand, the compiler optimization should be able to eliminate the redundant calls in current version. So the number of `getKind()` and comparison should be able to be reduced. (I know there is specific optimization for switch, but the cases is too small so that there is no big optimization space.)

On the other hand, the switch version would require much more lines of code, a draft of it would be:
```
switch(D->getKind()) {
   case TemplateTypeParmDecl:
       auto *TTP = cast<TemplateTypeParmDecl>(D);
       return TTP->hasDefaultArgument() &&
                      !TTP->defaultArgumentWasInherited();
   case ....
}
```

I feel it is worse. And I can't believe the performance here would be significant. Premature optimization is the root of all evil.

For the loop switching optimization, as I said, there number of calls to `getKind()` wouldn't be larger. On the other hand, I don't think it is good to do loop switching here for the judgement of getKind()... It is not worthy. The loop is relatively big and the comparison is relatively cheap. It would pay more to do loop unstitching for this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118437/new/

https://reviews.llvm.org/D118437