Yes you have. Sorry for the noise.
I don't think that's a good way to write a test like this. I suggest checking out test/Shell/SymbolFile/DWARF/DW_OP_piece-struct.s for an dwarf expression test that does not rely on guessing the DW_OP opcodes used by the compiler. Maybe you can just take that test and tweak it to do what you need?
Hmm. I would argue that the tuple discussion makes less sense in the context of this particular rewriting (since it really deals with getting rid of extract_slices/insert_slices using other slice ops). Cleaning up tuple uses is a nice bonus, but not a requirement, since tuples are really a part of the vector dialect to start with, as the leaking example shows. If you want something that blocks any tuple uses, I would argue that we need another pattern population for that, and run that at places where we need to guarantee they are gone (such as the lowering to LLVM, although the "legality" part takes care of it there already).
Sorry, I take my acceptance back. I do not think this is required. If you rebase to HEAD of master, it should work without this change. I think, when you reverted the changes to LLVMLibCRules.cmake, you removed LLVMSupport from the list of link libraries for add_libc_unittest targets.
Thanks @djtodoro for looking at this. Sorry for this really delayed response!
This was initially going to be a ping, but as a result of some light offline
discussion I have a few questions. For clarification an "overlap" here refers to
a partial overlap between fragments in DIExpressions.
Well my point here is that if you added the patterns to fold the tuples, you would do as good a job as you could in the absence of region/function boundaries.
Here you created an example that leaks in an unfixable way (because we have conciously decided to no lower the tuple<vector> type to LLVMIR and take the strong position that it must canonicalize/fold/DCE away).
Thanks for the fix.
Here is a proposal: we add two children to -Wrange-loop-analysis.
The follow-up is here: https://reviews.llvm.org/D73029 .
I have the need to change the ascending traversal implemented in ASTContext.cpp with a map (populated while descending) to make it skip nodes too during parent traversal.
I didn't want to have descending traversal in Expr.cpp and ascending traversal in ASTContext.cpp as they would be likely to go out of sync, so moving this implementation here allows extending the purpose in the follow-up.
The ascending traversal should not be in the AST library at all. If tooling or static analysis wants this, that's fine, but navigating upwards is not something the AST supports, and we do not want any part of clang proper developing a reliance on having a parent map, so we're working on moving the parent map out of ASTContext (with a plan to eventually move it out of the AST library). See D71313 for related work in this area.
The MBFIWrapper change seems NFC, can it be extracted out first?
LLVM already infers noalias nonnull for eg. _Znwm so noalias and nonnull info added by clang will not increase power of LLVM. Or?
To be honest, i don't really believe it is LLVM place to deduce attributes on standard library functions like these, frontend should be doing that.
Because as noted in patch description, if noalias is blindly "deduced" by LLVM,
that is a miscompile if -fno-assume-sane-operator-new was specificed.
From the commit message, this sounds like it is fixing a bug. Can it be tested? Did you have a test already, or did you forget to add it?
Otherwise, code looks good.
Also move the ashr/lshr and trunc patterns into freelyNegateValue() and keep existing handling regarding one-use checks.