User Details
- User Since
- Feb 10 2016, 9:51 AM (257 w, 5 d)
Thu, Jan 14
Do you have an idea if there is overhead of adding the AAR object to each query (each query info object)? (vs the setAAResults being done once for all the AAs available).
Wed, Jan 13
lgtm.
Runtime impact results look good.
Thank you for confirming @SjoerdMeijer.
Tue, Jan 12
Could you test clang as well, and update test?
I'm curious if the compile time impact is complemented by impact in run time. Looking into that for O3.
Mon, Jan 11
lgtm.
Thu, Jan 7
The changes LGTM, but I'd rather defer final review to ahatanak@.
It would be good to have some testing but it could be addressed in a separate patch, potentially by folks more familiar with the ObjCARC passes.
Wed, Jan 6
lgtm.
Looks reasonable to me as well, that the metadata info should hold at the same loop level.
Tue, Jan 5
Apologies for the delay, I spent some more time understanding how the assumptions changed in the new implementation.
I'll look into regenerating the bitcode files, but in the mean time this looks good.
Wed, Dec 30
Thank you for pushing the fix. Based on a quick look, it seems reasonable to not do the renaming for unreachable blocks. I'll look in more depth next week.
Wed, Dec 23
Thanks!
Mon, Dec 21
This seems like a useful utility to add to BasicBlock itself if it doesn't exit? (e.g. getNonEmptyName or getNameOrAsOperand).
Address comments.
Dec 16 2020
Many thanks!
Dec 15 2020
We've bisected a test failure to this revision.
Dec 14 2020
Dec 11 2020
Dec 10 2020
Thank you for adding this support.
Dec 9 2020
Dec 8 2020
Fix clang-tidy warning (lowercase method names; the file needs a lot of other functions names fixed, but might as well start with these)
Discussed this offline, and proposed this solution to make progress and resolve the failing tests.
This does need exploring further though as:
- the behavior of current LSR will no longer be tested in the new pass manager, due to LCSSA being run as a canonicalization pass.
- adding it to a loop pass manager in the npm will be problematic, because it doesn't preserve LCSSA.
Dec 7 2020
lgtm.
According to: https://clang.llvm.org/docs/AttributeReference.html#nomerge, the existance of the attribute should prevent call site merging, so the change makes sense.
However, in the test example then, if there was branching and two @f calls inside @bar, it would be correct to merge the @f call sites (no attribute preventing that inside) when processing @bar. But after inlining, that's no longer allowed if the attribute is distributed to all callsites inside the function.
This is a contradiction that needs clarification from folks more familiar with the Inliner.
Dec 1 2020
A few comments inline.
Nov 30 2020
Yes.
Nov 25 2020
Nov 24 2020
EarlySimplificationEPCallback?
Unblocking the fixing of the tests under NPM. Additional changes can be done as follow ups.
This is great, thank you for this fix!
sgtm too.
Ack.
lgtm.
Please wait in case fhahn@ has additional comments.
Nov 23 2020
One more comment, the others look good.
I don't think "ModuleOptimizer" in the name makes sense, the association can come from the "Early" part. Either keep the naming as is, or "EarlyAfterStart" if you want to define order against the other callbacks.
ychen@ so you have a better naming suggestion?
Thank you for the cleanup!
Nov 20 2020
Nov 19 2020
Algorithm lgtm.
+ @kuhar for additional review.
Nov 18 2020
Nov 17 2020
This makes sense to me too. The additional comment was helpful with the gcd/difference condition.
- LocationSize::unknownMaybeNegative() seems somewhat ambiguous in isolation; unless I see the other name or have the context from this patch. I'm either keep LocationSize::unknown or make it even more verbose LocationSize::unknownPositiveOrNegative(). I think the most restrictive case ( LocationSize::unknownMaybeNegative() or renamed version) should be the default ~uint64_t(0), but don't feel strongly about it.
- clang-format
Nov 16 2020
No objections.
As talked offline, this lgtm.
Could you clarify why a map and not a SmallVector?
Nov 13 2020
Nov 12 2020
I wouldn't be surprised if there are other asymmetries, thank you for digging further here.
Nov 11 2020
AFAICT all issues were addressed.
@Meinersbur: are there any concerns left with this patch?
Nov 10 2020
This is great, thank you!
Nov 9 2020
Nov 5 2020
Nov 4 2020
Resulting dot file for the test looks good.
Thank you for the fix.
lgtm.
Looking forward to having it enabled in a future patch.
Nov 3 2020
lgtm
Thank you for detailed tests, this looks good!
Thank you, just a couple of small comments below.
Nov 2 2020
Thank you for working on this, the compile times look very promising and the tests results are great!
The current behavior is:
- for the NPM, always use MSSA for LoopSink, which is a Function pass.
- for the LPM, LoopSink is a Loop pass and it will use MSSA conditioned on the EnableMSSALoopDependency flag which is set to true, hence this patch is switching loop sink to use MSSA.
This looks like a nice approach to clean up the recursion, thank you for working on this!
Oct 30 2020
I guess my question was more regarding consistency. Why not use MSSA in the LPM as well and drop the AST usage entirely? Or provide a flag that can be used to switch between MSSA and AST version (see LICM or DSE for similar flags using MSSA or alternative analyses), just in case there are users of the AST version.
Thank you!
Oct 29 2020
- Could you separate the SinkHoistFlag refactoring into an NFC?
- Could you clang-format and fix lints?
- LICM changes look good. AFAICT the MSSA changes in LoopSink are accurate, but it may need more testing than the existing unit tests.
- Why do you want the legacy pass manager to use the AST and the new pass manager MSSA?
I checked in the changes adding AAQI as authored by you.
The change clearing the cache is not right, as discussed on llvm-dev, so I think this patch is not obsolete.
Neat!
lgtm
This LGTM.
I'd like to understand better what you mean by not calling partialOverwrite. If the functionality in that call would be included in the isOverwrite call, note that there are call-paths that would do more work than they do now, and I thought there was a previous refactoring done to avoid this.
I may be mis-understanding though, so feel free to send the additional changes and we can discuss on those.
Oct 28 2020
This is an interesting point. IIUC, your argument is that the entry point into AA is always through AAResults and can rely on the invocation result for TBAA or ScopedAA from there, while the recursive invocations into these same analyses cannot yield stricter results. I cannot think of a case when this is not true.