- User Since
- Aug 19 2016, 10:21 AM (121 w, 4 d)
Wed, Dec 12
Committed in r348995. (I also took the liberty of adding a [Support] tag to the title and wrapping the commit message, in line with LLVM conventions; see https://llvm.org/docs/DeveloperPolicy.html#commit-messages). Thanks for the patch!
That's the intent ... it uploads the entire file to Phabricator, so that it can be viewed through the web interface. The arcanist tool does that for you automatically, as an alternative to have to copy and paste a giant block of text around.
This LGTM, but before I approve, I'd like to look around the surrounding code to confirm things are how I expect, but this patch is uploaded without context :( Could you reupload with context? See https://llvm.org/docs/Phabricator.html (the part about using -U999999 in your git commands to generate full context).
I started playing with this:
Tue, Dec 11
https://reviews.llvm.org/D55543 does the same thing
I'd tried this exact same patch back in https://reviews.llvm.org/D44619, but I was running into a bunch of check-clang failures with it, and I was never able to figure them out. It looks like it works now though, so I'm glad :) Richard's comment from that diff might still be relevant:
Mon, Dec 3
Fri, Nov 30
Thu, Nov 29
Straightforward fix, so LGTM.
Wed, Nov 21
Nov 15 2018
check-cxx-abilist passes for me with this applied (Mojave, Xcode 10.1).
Nov 14 2018
Nov 13 2018
@rnk pointed out on IRC that the MicrosoftCXXNameMangler is actually specifically designed to manage the mangling of only a single name, in which case adding state to it for handling RTTI seems like a natural approach. @rjmccall, what do you think? I think this is much cleaner than having to thread through the RTTI state to every individual method. The ForRTTI_t enum is modeled after the ForDefinition_t enum used in CGM, but I'm happy to switch to a more general struct (as you'd mentioned before) if you prefer.
LGTM, though you may wanna also wait for Zach.
Nov 9 2018
Nov 8 2018
From the summary,
Nov 7 2018
Sorry, had a leftover draft which I forgot to clean up. Edited in Phabricator now.
That's fair. I can just change the policy for clang explicitly in a separate diff then.
Somewhat orthogonal to this diff, but should we change clang so that it only does its own cmake_minimum_required call when it's being built standalone? There's potential for inconsistent policies between building clang in-tree and standalone if we do that, but it also seems strange to just wholesale ignore LLVM's policy settings.
Nov 6 2018
Nov 5 2018
I'm not familiar enough with this part of the code to feel comfortable accepting, sorry.
Nov 2 2018
I notice that you're using LLVM_BUILD_INSTRUMENTED=IR, which corresponds to -fprofile-generate (IR-level profiling), instead of -fprofile-instr-generate (clang-level profiling). Did you play around with both and observe that IR-level profiling gave you better results?
Oct 31 2018
LLVM's CMake has built in support for PGO; see https://llvm.org/docs/AdvancedBuilds.html#multi-stage-pgo. I haven't looked at the script in detail, but does it function similarly?
Oct 30 2018
Awesome, I don't have to maintain an internal patch for this anymore :)
Oct 25 2018
Oct 23 2018
Oct 22 2018
@rjmccall I prototyped the ForRTTI parameter approach in D53546. It could definitely be cleaned up a bit, but it demonstrates the problems I saw with the added parameter. Namely, mangleType(QualType, SourceRange, QualifierMangleMode) has a bunch of additional logic which is needed for correctness, so we need to thread the parameter through the entire chain, and the only way I could think of doing that without adding the parameter to every single mangleType overload was to have an additional switch to dispatch to the overloads with the added ForRTTI parameter, which is pretty ugly.
Oct 19 2018
Do you have real world performance numbers (both time and memory) for this? Computational complexity isn't always the best predictor of actual performance because of e.g. cache characteristics (as @Bigcheese mentioned).