davide (Davide Italiano)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 12 2014, 1:58 PM (140 w, 2 d)

Recent Activity

Yesterday

davide added a comment to D35741: Add MemorySSA alternative to AliasSetTracker in LICM..

Thanks, I'll review this soon.
A quick question. a concern people have about MemorySSA is that you pay upfront for O(1) queries, which may have some impact on compile time.

FWIW: This is and always has been false accounting in most passes as you know.
Most passes that could use MemorySSA already ask about every load and store in the function.
The only reason people think it's faster to not use MemorySSA, or to avoid the upfront cost, is because our time-passes doesn't account function time to lazy utilities/analysis properly.
If it did, AA/etc time would look *much* larger than it does now, because in reality, it *is* much larger than it seems.

IE if you avoid optimize uses, and call getClobberingAccess on every load/store, "MemorySSA" time goes down, overall time goes up.

Fri, Jul 21, 5:32 PM
davide added a comment to D35741: Add MemorySSA alternative to AliasSetTracker in LICM..

Thanks, I'll review this soon.
A quick question. a concern people have about MemorySSA is that you pay upfront for O(1) queries, which may have some impact on compile time.
(e.g. in EarlyCSE). AST is already O(N^2), and in some cases badly so, but I wonder if you have compile time measurements of the impact of your change?

Fri, Jul 21, 3:47 PM

Thu, Jul 20

davide committed rL308668: [PGO] Move the PGOInstrumentation pass to new OptRemark API..
[PGO] Move the PGOInstrumentation pass to new OptRemark API.
Thu, Jul 20, 1:46 PM
davide added a comment to D33539: [PM/PGO] Wire up the opt driver's new PM support to be able to run PGO..

Trying to catch up on reviews.
Chandler, was this ever committed?

Thu, Jul 20, 10:22 AM

Wed, Jul 19

davide added inline comments to D35609: [LICM] Make sinkRegion and hoistRegion non-recursive.
Wed, Jul 19, 6:27 PM
davide added a comment to D35609: [LICM] Make sinkRegion and hoistRegion non-recursive.

LGTM, nice to have you back from the grave :)

Wed, Jul 19, 6:21 PM
davide added a comment to D35639: [LTO] Prevent dead stripping and internalization of symbols with sections.
In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

Wed, Jul 19, 2:49 PM
davide committed rL308525: [TRE] Add another test for OptRemark..
[TRE] Add another test for OptRemark.
Wed, Jul 19, 2:40 PM
davide added a comment to D35639: [LTO] Prevent dead stripping and internalization of symbols with sections.

Do you know whether LTO gets this case right?

It does not get it correct. I could move the fix into the internalize handling under llvm::internalizeModule: that would cover ThinLTO with the new LTO API, and ThinLTO and regular LTO with the legacy LTO API. However, the regular LTO handling in LTO::runRegularLTO does not use internalizeModule. I would either need to duplicate the fix for regular LTO into both the new and legacy LTO handling, or move the fix into internalizeModule and refactor runRegularLTO to use it as well. WDYT?

Wed, Jul 19, 2:34 PM
davide added a comment to D35570: [ORE] Port TailRecursionElimination to the new API.

Addressed your comments & committed.

Wed, Jul 19, 2:14 PM
davide committed rL308524: [TRE] Move to the new OptRemark API..
[TRE] Move to the new OptRemark API.
Wed, Jul 19, 2:14 PM
davide closed D35570: [ORE] Port TailRecursionElimination to the new API by committing rL308524: [TRE] Move to the new OptRemark API..
Wed, Jul 19, 2:13 PM
davide added a comment to D35570: [ORE] Port TailRecursionElimination to the new API.

LGTM once we clear the question below. Thanks!

Wed, Jul 19, 2:08 PM
davide added inline comments to D35570: [ORE] Port TailRecursionElimination to the new API.
Wed, Jul 19, 2:02 PM
davide accepted D35534: [opt-viewer] Reduce memory consumption.

LGTM.

Wed, Jul 19, 1:51 PM
davide added a comment to D35639: [LTO] Prevent dead stripping and internalization of symbols with sections.

Do you know whether LTO gets this case right?

Wed, Jul 19, 1:35 PM
davide added inline comments to D35570: [ORE] Port TailRecursionElimination to the new API.
Wed, Jul 19, 1:19 PM
davide added inline comments to D35570: [ORE] Port TailRecursionElimination to the new API.
Wed, Jul 19, 1:18 PM
davide updated the diff for D35570: [ORE] Port TailRecursionElimination to the new API.

@anemet, this should be ready for review.

Wed, Jul 19, 1:12 PM
davide committed rL308503: [X86] Don't try to scale down if that exceeds the bitwidth..
[X86] Don't try to scale down if that exceeds the bitwidth.
Wed, Jul 19, 11:13 AM
davide added a comment to D35609: [LICM] Make sinkRegion and hoistRegion non-recursive.

If I understand the patch correctly, aren't we just iterating children->parent, therefore the order of siblings should be irrelevant in this case ?
(i.e. the worklist is sorted according to the dominance relation and shouldn't really matter if we process two siblings in the dominator in different order?)
I think Sanjoy's suggestion works if my reasoning is correct, although I'm also fine with the current version of the patch (I don't have a strong preference for one or the other).

Wed, Jul 19, 10:41 AM
davide requested changes to D35608: Migrate SimplifyLibCalls to new OptimizationRemarkEmitter.

Please add a test exercising this codepath (you can take inspiration from the ones in GVN or LV)

Wed, Jul 19, 8:11 AM

Tue, Jul 18

davide added a comment to D35609: [LICM] Make sinkRegion and hoistRegion non-recursive.

The idea is sound, I'll think a little bit more about the visitation order (tomorrow morning) but it seems you got it right.
Just a minor nit, I see this patch has some unrelated formatting, mind to revert that? (and/or commit separately)

Tue, Jul 18, 10:45 PM
davide added a comment to D35606: Move TailRecursionElimination to new OptimizationRemarkEmitter API.

I have just discovered that I have a failing test, in test/Other/new-pm-thinlto-defaults.ll Yours will probably fail there too.

@davide I'll defer to you and your patch on this, given you have comments etc. No worries from me about my duplicated effort.

Tue, Jul 18, 9:43 PM
davide added a comment to D35606: Move TailRecursionElimination to new OptimizationRemarkEmitter API.

Bummer, looks like you and @davide both worked on the same pass: D35570 and came up with very similar patches. @davide did you make progress since the last rev?

Tue, Jul 18, 9:20 PM
davide added a comment to D35606: Move TailRecursionElimination to new OptimizationRemarkEmitter API.

This closes https://bugs.llvm.org/show_bug.cgi?id=33788

This commit almost certainly needs work, I'm new to the llvm codebase. I also don't fully understand arc, I think I have the right lists subscribed and have asked for reviews from the right people, but I'm not sure.

Tue, Jul 18, 9:15 PM
davide accepted D35528: [Dominators] Teach LoopUnswitch to use the incremental API.

The logic is fine. Thanks!

Tue, Jul 18, 11:27 AM
davide created D35570: [ORE] Port TailRecursionElimination to the new API.
Tue, Jul 18, 9:56 AM
davide added inline comments to D35568: [AArch64] Use 16 bytes as preferred function alignment on Cortex-A53..
Tue, Jul 18, 9:46 AM
davide added inline comments to D35568: [AArch64] Use 16 bytes as preferred function alignment on Cortex-A53..
Tue, Jul 18, 9:19 AM
davide committed rL308319: [TRE] Simplify canTRE() a bit using all_of(). NFCI..
[TRE] Simplify canTRE() a bit using all_of(). NFCI.
Tue, Jul 18, 8:43 AM

Mon, Jul 17

davide added a comment to D35528: [Dominators] Teach LoopUnswitch to use the incremental API.

I'm a little fried at the moment, so just some drive-by comments. I wonder if we can make the memory management automatic instead of calling naked delete.

Mon, Jul 17, 9:16 PM
davide added a comment to D35491: [opt-viewer] Accept directories that are searched for opt.yaml files.

sgtm

Mon, Jul 17, 10:47 AM
davide accepted D35491: [opt-viewer] Accept directories that are searched for opt.yaml files.

LGTM with the small nits addressed.

Mon, Jul 17, 10:34 AM
davide added inline comments to D35491: [opt-viewer] Accept directories that are searched for opt.yaml files.
Mon, Jul 17, 10:05 AM
davide added inline comments to D35491: [opt-viewer] Accept directories that are searched for opt.yaml files.
Mon, Jul 17, 9:57 AM

Sun, Jul 16

davide accepted D33608: [Analysis] RemoveTotalMemInst counting in InstCount to avoid reading back other Statistic variables.

I'm with Chandler on this one.

Sun, Jul 16, 9:55 PM
davide committed rL308144: [InstCombine] Don't violate dominance when replacing instructions..
[InstCombine] Don't violate dominance when replacing instructions.
Sun, Jul 16, 11:57 AM
davide closed D35376: [InstCombine] Don't violate dominance when replacing instructions by committing rL308144: [InstCombine] Don't violate dominance when replacing instructions..
Sun, Jul 16, 11:57 AM

Fri, Jul 14

davide committed rL308055: [AMDGPU] Throw away more dead code. NFCI..
[AMDGPU] Throw away more dead code. NFCI.
Fri, Jul 14, 2:21 PM
davide requested changes to D35437: Don't break bundles when adding DBG_VALUE.

This change has no test associated. Can you please add one?
Also, this patch is oddly formatted, can you please run clang-format on it?

Fri, Jul 14, 1:22 PM
davide added a comment to D35317: [EarlyCSE] Handle calls with no MemorySSA info..

I'm going to go ahead with this version of the fix (with the comments addressed). I implemented the stronger version (i.e. looking at MSSA memory accesses when checking doesNotAccessMemory, onlyReadsMemory, mayReadFromMemory, mayWriteMemory) and saw a total of 2 differences in the values returned from these checks across all of our benchmarks, and no change in code generation at all. Given the amount of code churn to implement this and how close we are to release, I'm going to take the conservative route.

Fri, Jul 14, 12:33 PM
davide committed rL308047: [AMDGPU] Garbage collect dead code. NFCI..
[AMDGPU] Garbage collect dead code. NFCI.
Fri, Jul 14, 11:48 AM
davide added inline comments to D35376: [InstCombine] Don't violate dominance when replacing instructions.
Fri, Jul 14, 8:36 AM
davide added inline comments to D35376: [InstCombine] Don't violate dominance when replacing instructions.
Fri, Jul 14, 8:35 AM
davide accepted D35286: [Dominators] Simplify block and node printing.

LGTM, thanks.

Fri, Jul 14, 8:31 AM
davide added a comment to D35317: [EarlyCSE] Handle calls with no MemorySSA info..

This fix is basically correct, I'm fine with this going in after Dan's comments are addressed.
I don't have strong opinions on the alternative solution suggested by Vedant, so up to you.

Fri, Jul 14, 8:31 AM

Thu, Jul 13

davide added a comment to D35376: [InstCombine] Don't violate dominance when replacing instructions.

Forgot to mention. This fixes https://bugs.llvm.org/show_bug.cgi?id=33765

Thu, Jul 13, 12:14 PM
davide created D35376: [InstCombine] Don't violate dominance when replacing instructions.
Thu, Jul 13, 12:13 PM
davide accepted D35282: [Dominators] Split SemiNCA into smaller functions.

Looks good to me as well (agree that the machinecombiner bit should be separated, you can do that without pre-commit review, it's a no-brainer [assuming it doesn't break anything :)]

Thu, Jul 13, 10:31 AM
davide committed rL307921: [GlobalOpt] Autogenerate checks for the test in PR33686..
[GlobalOpt] Autogenerate checks for the test in PR33686.
Thu, Jul 13, 8:56 AM
davide committed rL307920: Reapply [GlobalOpt] Remove unreachable blocks before optimizing a function..
Reapply [GlobalOpt] Remove unreachable blocks before optimizing a function.
Thu, Jul 13, 8:41 AM
davide accepted D35337: [Solaris] gcc runtime dropped support for .ctors, switch to .init_array.

LGTM.

Thu, Jul 13, 8:17 AM

Wed, Jul 12

davide committed rL307880: [opt-viewer] Don't except when debug info is not available..
[opt-viewer] Don't except when debug info is not available.
Wed, Jul 12, 9:19 PM
davide committed rL307874: [sanstats] Remove a flaky test..
[sanstats] Remove a flaky test.
Wed, Jul 12, 6:36 PM
davide committed rL307871: [sanstats] Print the correct line information..
[sanstats] Print the correct line information.
Wed, Jul 12, 5:49 PM
davide added a comment to D35286: [Dominators] Simplify block and node printing.

This is not your fault but it would be awesome if we introduced a common error handling for the dom, so that we can just call that routine which also calls flush.

Is there something similar in LLVM that I could use as an example?

Wed, Jul 12, 4:30 PM
davide added a comment to D35286: [Dominators] Simplify block and node printing.

This is not your fault but it would be awesome if we introduced a common error handling for the dom, so that we can just call that routine which also calls flush.
(that way we don't have to repeat flush all over the place). Maybe in a separate patch.

Wed, Jul 12, 4:20 PM
davide committed rL307797: [X86/FastIsel] Fall-back to SelectionDAG when lowering soft-floats..
[X86/FastIsel] Fall-back to SelectionDAG when lowering soft-floats.
Wed, Jul 12, 8:26 AM
davide closed D35272: [X86/FastIsel] Fall-back to SelectionDAG when lowering soft-floats by committing rL307797: [X86/FastIsel] Fall-back to SelectionDAG when lowering soft-floats..
Wed, Jul 12, 8:26 AM
davide added inline comments to D35282: [Dominators] Split SemiNCA into smaller functions.
Wed, Jul 12, 8:19 AM
davide accepted D35285: [Dominators] Simplify templates.

LGTM

Wed, Jul 12, 8:17 AM

Tue, Jul 11

davide committed rL307729: [IPO] Temporarily rollback r307215..
[IPO] Temporarily rollback r307215.
Tue, Jul 11, 4:10 PM
davide added a comment to D35277: Make shell redirection construct portable.

This is good for portability but probably needs to be specified somewhere (programmer's manual?) but I'm not sure if we have a section on how to write tests.
Also, is there a netbsd bot to catch mistakes in future? [I don't see it].

Tue, Jul 11, 2:44 PM
davide created D35272: [X86/FastIsel] Fall-back to SelectionDAG when lowering soft-floats.
Tue, Jul 11, 1:10 PM
davide committed rL307699: [NewGVN] Check for congruency of memory accesses..
[NewGVN] Check for congruency of memory accesses.
Tue, Jul 11, 12:49 PM
davide committed rL307694: [NewGVN] Fix an innocent typo I found while debugging PR33720..
[NewGVN] Fix an innocent typo I found while debugging PR33720.
Tue, Jul 11, 12:20 PM
davide committed rL307692: [NewGVN] Clarify the function invariants formatting them properly..
[NewGVN] Clarify the function invariants formatting them properly.
Tue, Jul 11, 12:16 PM

Mon, Jul 10

davide committed rL307586: [NewGVN] Simplify a lambda a little bit. NFCI..
[NewGVN] Simplify a lambda a little bit. NFCI.
Mon, Jul 10, 1:45 PM

Sun, Jul 9

davide added inline comments to D34639: Fix DebugLoc propagation for unreachable LoadInst.
Sun, Jul 9, 5:32 PM
davide committed rL307508: [X86] Relax an assertion when legalizing vector types..
[X86] Relax an assertion when legalizing vector types.
Sun, Jul 9, 12:23 PM
davide added inline comments to D35142: WIP! [CFG] Create a new removeUnreachable utility that updates the dom in place.
Sun, Jul 9, 9:43 AM
davide committed rL307425: [Local] Update the comment for removeUnreachableBlocks..
[Local] Update the comment for removeUnreachableBlocks.
Sun, Jul 9, 6:16 AM
davide updated subscribers of D35142: WIP! [CFG] Create a new removeUnreachable utility that updates the dom in place.
Sun, Jul 9, 6:13 AM
davide added a comment to D35028: [GlobalOpt] Remove unreachable blocks before optimizing a function.

Actually, thinking about this more, i'm confused.

removeUnreachableBlocks does silly things (it computes liveness and blah blah blah blah) and modifies the CFG, but those silly things are irrelevant to the problem here.

The following should suffice to fix these bugs without explicitly recalculating the dom tree:

Walk blocks using  RPO iterator, put them in Live set

For each block in function not in Live:
  if (Processed.count(X)) skip it.

  for X in depth_first (DT->getNode(block)):
       Processed.insert(X)
        for (BasicBlock *Successor : successors(X))
          Successor->removePredecessor(X);
        if (LVI)
          LVI->eraseBlock(X);
        DT->eraseBlock(DT->getNode(X))
        X->dropAllReferences();
        X->eraseFromParent();

You have to erase the blocks in the dom tree leaves first.

Otherwise, this is just "find forward unreachable blocks, delete them" (the code is from removeUnreachableBlocks)

By definition, any block dominated by an unreachable block is also forward unreachable, so we just process them as part of the depth first walk.

Sun, Jul 9, 6:13 AM
davide created D35142: WIP! [CFG] Create a new removeUnreachable utility that updates the dom in place.
Sun, Jul 9, 6:13 AM
davide updated subscribers of D35028: [GlobalOpt] Remove unreachable blocks before optimizing a function.
Sun, Jul 9, 6:13 AM

Fri, Jul 7

davide requested changes to D34080: [ThinLTO] Add dump-summary command to llvm-lto2 tool.

Please hold the press on this one for a bit.
I didn't see @chandlerc / @dblaikie comments on the RFC and they seem to not have been fully addressed.
I guess you may want to continue the discussion there before coming back to this.

Fri, Jul 7, 10:05 AM
davide committed rL307412: [LTO] Add a test for ThinLTO + --defsym..
[LTO] Add a test for ThinLTO + --defsym.
Fri, Jul 7, 9:40 AM
davide committed rL307410: [LTO] Add a test for ThinLTO + --wrap..
[LTO] Add a test for ThinLTO + --wrap.
Fri, Jul 7, 9:33 AM
davide closed D35126: [LTO] Add a test for ThinLTO + --wrap by committing rL307410: [LTO] Add a test for ThinLTO + --wrap..
Fri, Jul 7, 9:33 AM
davide added a comment to D35044: [IR] Remove the opcode argument from CmpInst::Create.

This is correct, but why?

Fri, Jul 7, 9:27 AM
davide added a comment to D34080: [ThinLTO] Add dump-summary command to llvm-lto2 tool.

Teresa, any further comments or we can go ahead and commit Charles' patch?

Fri, Jul 7, 9:24 AM
davide added a reviewer for D35126: [LTO] Add a test for ThinLTO + --wrap: ruiu.
Fri, Jul 7, 9:22 AM
davide created D35126: [LTO] Add a test for ThinLTO + --wrap.
Fri, Jul 7, 9:20 AM

Thu, Jul 6

davide added inline comments to D35074: [LoopRotate] Fix DomTree update logic for unreachable nodes. Fix PR33701..
Thu, Jul 6, 2:20 PM
davide committed rL307305: [lib/LTO] Add a comment to explain where we set the linkage in the summary..
[lib/LTO] Add a comment to explain where we set the linkage in the summary.
Thu, Jul 6, 1:04 PM
davide committed rL307303: [LTO] Fix the interaction between linker redefined symbols and ThinLTO.
[LTO] Fix the interaction between linker redefined symbols and ThinLTO
Thu, Jul 6, 12:59 PM
davide closed D35064: [lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO by committing rL307303: [LTO] Fix the interaction between linker redefined symbols and ThinLTO.
Thu, Jul 6, 12:59 PM
davide updated the diff for D35064: [lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO.

Thanks for the comments, Peter & Teresa. New version available.

Thu, Jul 6, 12:01 PM
davide added inline comments to D35064: [lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO.
Thu, Jul 6, 11:41 AM
davide updated the diff for D35064: [lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO.

Actually, I now think I understand this slightly better.
As this was handling only the update of linkage to available_externally, I think the easiest solution is that of moving the early return later and in case the original and the linkage in the summary don't match, and the one in the summary is weak, just switch to it.
I think this is safe, but maybe we might want to do the switch only for some kind of (original) linkages, maybe.

Thu, Jul 6, 10:52 AM
davide planned changes to D35064: [lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO.

This has a known problem, I'll upload a new version in a bit.

Thu, Jul 6, 10:13 AM
davide created D35064: [lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO.
Thu, Jul 6, 9:17 AM
davide accepted D35048: [opt-viewer] Move under tools, install it.

I'm okay with or without the llvm-prefix.
I think it's useful for tools that have a GNU equivalent, but this one hasn't one, so, LGTM.

Thu, Jul 6, 8:06 AM

Wed, Jul 5

davide added a comment to D35028: [GlobalOpt] Remove unreachable blocks before optimizing a function.
for X in depth_first (DT->getNode(block)):

the depth_first here should be post_order, i forgot depth_first is preorder

Wed, Jul 5, 3:31 PM
davide committed rL307215: [GlobalOpt] Remove unreachable blocks before optimizing a function..
[GlobalOpt] Remove unreachable blocks before optimizing a function.
Wed, Jul 5, 3:29 PM
davide closed D35028: [GlobalOpt] Remove unreachable blocks before optimizing a function by committing rL307215: [GlobalOpt] Remove unreachable blocks before optimizing a function..
Wed, Jul 5, 3:29 PM
davide updated the diff for D35028: [GlobalOpt] Remove unreachable blocks before optimizing a function.

Updated after Eli's feedback. I'll change analyzeGlobals to not revisit instructions as a follow up if you're okay with it.
Thanks for taking the time to review this change.

Wed, Jul 5, 2:24 PM
davide added inline comments to D35028: [GlobalOpt] Remove unreachable blocks before optimizing a function.
Wed, Jul 5, 1:44 PM