- User Since
- Feb 10 2016, 9:51 AM (206 w, 4 d)
Fri, Jan 24
Thu, Jan 23
Wed, Jan 22
Oh, wow! Might I ask you add the same for fmul? We may get a revision like that next time :).
Tue, Jan 21
Thanks you for taking this on!
Fri, Jan 17
Thu, Jan 16
Nit: remove F.
Add comment explaining invalidate rational.
GlobalsAA is stateless and is preserved, unless it's explicitly invalidated.
Not a bug fix, but a prerequisite for a change I'm working on for the new pass manager, requiring all passes to have an invalidate method.
I sent it for review to get another pair of eyes on the invalidate implementation in case I missed something.
Wed, Jan 15
Regarding handling the branching example, the solution fhahn proposes sounds reasonable to me.
I was thinking something similar, along the lines of: do the checks bottom-up from both second and third stores during the main algorithm, both find the dead store but do not postdominate it; don't abandon them, but keep this info (include the checks for no intervening reads) for after the main search. The data structure could be a Hashmap<PotentialDeadStore, ListOfDefsOrBlocksWhoFoundThisPotentialDeadStore>. If all paths are covered in the List for a PotentialDeadStore, then it's truly dead.
Tue, Jan 14
Just a general comment here: DSE shouldn't necessarily make all optimizations possible. This one looks like it should be caught by DCE. If it's not, perhaps it would be good to improve DCE. Alternatively, if it's cheap enough, add a comment that it's still worth adding it to DSE.
Thanks, this looks useful!
Thank you for the other patch! I added a few comments on the bottom up approach. I think overall it's a better high-level approach.
Mon, Jan 13
Some comments inline.
Thu, Jan 2
Dec 23 2019
Dec 12 2019
Sorry, I forgot about this one, thank you for the reminder!
Dec 10 2019
lgtm, thank you!
Dec 6 2019
This revision appears to be the root-cause of https://bugs.llvm.org/show_bug.cgi?id=44231.
Dec 5 2019
Filed https://bugs.llvm.org/show_bug.cgi?id=44231 with a small reproducer.
This revision came back as the root-cause for an internal iOS crash.
It's hitting the following assertion:
assert.h assertion failed at llvm/lib/Target/ARM/ARMFrameLowering.cpp:468 in virtual void llvm::ARMFrameLowering::emitPrologue(llvm::MachineFunction &, llvm::MachineBasicBlock &) const: getMaxFPOffset(MF.getFunction(), *AFI) <= FPOffset && "Max FP estimation is wrong"
I have no expertise here, but this fixes PR44222 (many AArch64 internal crashes caused by D70673), so I'm going to go ahead and approve this to get the fix in tree.
Dec 3 2019
Thank you for this fix. LGTM.
Nov 25 2019
Nov 22 2019
Combine 3 methods.
Nov 21 2019
Nov 20 2019
Nov 13 2019
This looks like a great improvement, IMO.
Nov 12 2019
Nov 5 2019
Remove missed comment.
I updated the patch to be more generic, while still addressing this usecase.
The condition is: if there are any internal methods whose address is taken but for which we do not conclude that they do not modify globals, then cannot conclude lower than ModRef for any other (LocalLinkageCall, GlobalValue).
Nov 1 2019
Oct 31 2019
Oct 30 2019
I think this is a great example of a unit test actually.
Getting to the scenario of forcing ScEv to cache the Load, so we can verify it is no longer properly cached after the Load is hoisted is harder to do in a lit test such that it's so easy to understand, like it is in this unit test.
Oct 24 2019
Thank you for the patch! This LGTM as is.
Oct 23 2019
Since I don't really understand the CodeExtractor, I'm relying on the author or another reviewer to confirm the correctness here.
This patch by itself looks good, but please wait to see if there are additional comments for the other reviewers before checking in.
Oct 21 2019
Some comments, but it looks reasonable overall.
Oct 17 2019
Oct 16 2019
Oct 15 2019
Update iterator var names.
Nit: rename optional argument.
I'd like to check this in, in an effort to clear my patch queue. Please let me know if you have any comments.