davide (Davide Italiano)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 12 2014, 1:58 PM (148 w, 5 d)

Recent Activity

Yesterday

davide added a comment to D38005: [COFF] Check for sections larger than 4 GiB.

yaml2obj should allow you to create a broken object like that. IIRC there's an example in test/ELF.

Mon, Sep 18, 3:48 PM
davide committed rL313550: [ELF] Remove default argument for lambda..
[ELF] Remove default argument for lambda.
Mon, Sep 18, 11:33 AM

Fri, Sep 15

davide added a comment to D37933: Prevent InstCombine from miscompiling `operator delete` .

So, from what I can see no transformation made for free() is valid in case of operator delete.
What do you think instead of introducing a predicate isCallToOperatorDelete() and bail out at the beginning of visitFree() ?
Alternatively, we could add a boolean argument to isFreeCall() which doesn't include operator delete (still trying to pick a name for the argument).

Fri, Sep 15, 3:23 PM
davide added a comment to D37933: Prevent InstCombine from miscompiling `operator delete` .

Side note: instcombine speculates these calls to free using pattern matching.
In my opinion, it shouldn't. Hoisting shouldn't be done in instcombine, and hoisting shouldn't be done on a per-name basis. Presumably, the frontend should attach an attribute to this call to make sure they can be hoisted.
If there's not enough information in the frontend, then the middle-end should compute this information (the attribute) and then some pass should be responsible for the sinking/hoisting.

Fri, Sep 15, 2:46 PM
davide created D37933: Prevent InstCombine from miscompiling `operator delete` .
Fri, Sep 15, 2:42 PM
davide committed rL313394: [ConstantFold] Return the correct type when folding a GEP with vector indices..
[ConstantFold] Return the correct type when folding a GEP with vector indices.
Fri, Sep 15, 1:54 PM
davide closed D37928: [ConstantFold] Return the correct type when folding a GEP with vector indices by committing rL313394: [ConstantFold] Return the correct type when folding a GEP with vector indices..
Fri, Sep 15, 1:54 PM
davide added inline comments to D37928: [ConstantFold] Return the correct type when folding a GEP with vector indices.
Fri, Sep 15, 1:47 PM
davide created D37928: [ConstantFold] Return the correct type when folding a GEP with vector indices.
Fri, Sep 15, 1:37 PM
davide abandoned D37927: [ConstantFold] Return the correct type when folding a GEP with vector indices.
Fri, Sep 15, 1:34 PM
davide created D37927: [ConstantFold] Return the correct type when folding a GEP with vector indices.
Fri, Sep 15, 1:33 PM
davide closed D37837: Keep some relocations with undefined weak symbols.

r313372

Fri, Sep 15, 11:11 AM
davide updated subscribers of D37902: [CodeExtractor] Fix multiple bugs under certain shape of extracted region.

I'm a little nervous about updating the dominator manually, still. I wonder if we can use the new API that Jakub implemented (cc: @kuhar / @dberlin )

Fri, Sep 15, 9:12 AM
davide added reviewers for D37902: [CodeExtractor] Fix multiple bugs under certain shape of extracted region: davidxl, wmi.
Fri, Sep 15, 9:11 AM

Thu, Sep 14

davide accepted D37837: Keep some relocations with undefined weak symbols.

LGTM

Thu, Sep 14, 7:42 PM
davide added a comment to D37837: Keep some relocations with undefined weak symbols.

Thanks for fixing this.
I have no objections to the way you fixed it, some comments inline. Maybe Rui wants to take a look at the style.

Thu, Sep 14, 2:36 PM
davide added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

Added some unit tests. Thanks for the suggestion. I briefly considered moving SCCP over to the generic solver (as a simple way to re-purpose the existing tests), but this might not be that straightforward after all. The re-solving for undef values will need to be considered. In the end, adding unit tests for this patch wasn't that difficult to do.

Thu, Sep 14, 2:27 PM

Wed, Sep 13

davide updated subscribers of D37762: [InstCombine] Remove single use restriction from InstCombine's explicit sinking code..

Has there been any sort of discussion on expanding/using the existing IR level code sinking pass? I am referring to the SinkingPass in scalar/sink.cpp. AFAIK it's only used in the AMDGPU preisel pipeline. I don't know it's current state/usability but the description of the pass is:

This pass moves instructions into successor blocks, when possible, so that
they aren't executed on paths where their results aren't needed.

Doing this as a late IR pass makes sense. Taking a quick look at the implementation, there may be some improvements that would help (e.g., make it use MemorySSA, better handling of debug info, use a postdom tree). Considering adding this to the default codegen IR pipeline could be a good idea (and maybe better than trying to do a better job at the MI level?).

Wed, Sep 13, 3:17 PM

Tue, Sep 12

davide added reviewers for D37762: [InstCombine] Remove single use restriction from InstCombine's explicit sinking code.: sebpop, hiraditya.
Tue, Sep 12, 3:19 PM
davide added a comment to D37762: [InstCombine] Remove single use restriction from InstCombine's explicit sinking code..

I think what matters in this case is the {post}dominance relation between the block of the DEF and the block(s) [potentially > 1] of the USEs.
Doing this xform when all the uses are in the same block, is, correct, but restrictive. So, I think your logic is fine, but this makes me still less convinced that we shouldn't use the dom to drive this analysis (and therefore should be a separate pass :)

Tue, Sep 12, 3:17 PM

Mon, Sep 11

davide added a comment to D37714: llvm-dwarfdump: Replace -debug-dump=sect option with individual options..

The changes to llvm-dwarfdump/llvm-objdump look good to me.
I didn't review all the tests, but that seems like something you did with grep, so I wouldn't be too worried if it passes the suite.

Mon, Sep 11, 3:00 PM
davide added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

If it's not too hard, you may consider adding some unit tests for the sparse engine.

Ah, I didn't think about adding tests to unittests/. That makes sense - I'll give it a shot. I do wonder if it'd be easier to just move SCCP over to the generic engine, though.

Mon, Sep 11, 2:55 PM
davide added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

If it's not too hard, you may consider adding some unit tests for the sparse engine.

Mon, Sep 11, 2:21 PM

Fri, Sep 8

davide committed rL312836: [AMDGPU] Remove unused function. NFCI..
[AMDGPU] Remove unused function. NFCI.
Fri, Sep 8, 4:55 PM

Thu, Sep 7

davide added a comment to D37567: [llvm-readobj] - Teach tool to report error if some section is in multiple COMDAT groups at once..

We should really move away from a model where we check in a binary for the dumper output.
Can you try to write a YAML file to generate the same object? (I think mc won't do it).

Thu, Sep 7, 10:48 AM

Wed, Sep 6

davide added a comment to D37553: Analysis: Correctly handle all function operand references..

lgtm

Wed, Sep 6, 10:50 PM
davide committed rL312669: [ELF/Writer] Fix english in a comment. NFCI..
[ELF/Writer] Fix english in a comment. NFCI.
Wed, Sep 6, 2:18 PM
davide added inline comments to D37528: [JumpThreading] Preserve dominance across the pass..
Wed, Sep 6, 2:08 PM
davide accepted D37524: Detect linker script INCLUDE cycles..

Other than that, lg.

Wed, Sep 6, 11:06 AM
davide added inline comments to D37524: Detect linker script INCLUDE cycles..
Wed, Sep 6, 11:06 AM
davide accepted D37518: Fix PR33878: BasicAA incorrectly assumes different address spaces don't alias.

lg, thanks.

Wed, Sep 6, 9:53 AM

Tue, Sep 5

davide committed rL312579: [unittest/ReverseIteration] Unbreak when compiling with GCC..
[unittest/ReverseIteration] Unbreak when compiling with GCC.
Tue, Sep 5, 2:28 PM
davide committed rL312575: [GVNHoist] Move duplicated code to a helper function. NFCI..
[GVNHoist] Move duplicated code to a helper function. NFCI.
Tue, Sep 5, 1:51 PM

Mon, Sep 4

davide committed rL312501: [ABI] Rewrite RegisterIsCalleeSaved..
[ABI] Rewrite RegisterIsCalleeSaved.
Mon, Sep 4, 2:22 PM
davide closed D37420: [ABI] Rewrite RegisterIsCalleeSaved by committing rL312501: [ABI] Rewrite RegisterIsCalleeSaved..
Mon, Sep 4, 2:22 PM
davide added a comment to D37447: [Decompression] Fail gracefully when out of memory.

I don't think we want to do something fancier, at least for now.
If you run out of memory, we just report instead of crashing.
Presumably, if something shows up, we can have an API that propagates back errors to the caller which can take an appropriate action, but I don't see a compelling use case (maybe I miss one?)

Mon, Sep 4, 1:56 PM
davide added a comment to D36656: [SCCP] Propagate integer range information in IPSCCP (WIP)..

I think this is on the right direction (for me), but let's see what other think.
If we agree, it will probably make sense revamping the general semilattice engine to plug arbitrary lattices in (and move the constant semilattice out).
This will enable future improvements (i.e. support for IPA bitwise CP, see, e.g. https://patchwork.ozlabs.org/patch/657644/)

Mon, Sep 4, 12:37 PM

Sun, Sep 3

davide committed rL312457: [UUID] Reimplement comparison operators more canonically. NFCI..
[UUID] Reimplement comparison operators more canonically. NFCI.
Sun, Sep 3, 1:54 PM
davide added a reviewer for D37420: [ABI] Rewrite RegisterIsCalleeSaved: compnerd.
Sun, Sep 3, 12:36 PM
davide committed rL312454: [Interpreter] Simplify else after return. NFCI..
[Interpreter] Simplify else after return. NFCI.
Sun, Sep 3, 12:29 PM
davide committed rL312453: [Core/Value] Remove dead code that hasn't been touched in years. NFC..
[Core/Value] Remove dead code that hasn't been touched in years. NFC.
Sun, Sep 3, 12:26 PM

Sat, Sep 2

davide added a comment to D24882: Add StringSwitch::Cases functions that takes 6, 7 or 8 arguments..

I ended up in a situation where I needed 11 arguments (if you want a similar example, just grep for StringSwitch<bool> in MC).
I think the proposed solution was OK'ish up to 10 arguments, but adding more might be not ideal. So, now that we live in a post C++98 world, what do you think about switching this class to use variadic templating? I'm going to implement such a solution if nobody has objections. (cc: @dblaikie for thoughts)

Sat, Sep 2, 9:15 PM
davide added inline comments to D37420: [ABI] Rewrite RegisterIsCalleeSaved.
Sat, Sep 2, 9:15 PM
davide created D37420: [ABI] Rewrite RegisterIsCalleeSaved.
Sat, Sep 2, 9:14 PM
davide updated subscribers of D24882: Add StringSwitch::Cases functions that takes 6, 7 or 8 arguments..

I ended up in a situation where I needed 11 arguments (if you want a similar example, just grep for StringSwitch<bool> in MC).
I think the proposed solution was OK'ish up to 10 arguments, but adding more might be not ideal. So, now that we live in a post C++98 world, what do you think about switching this class to use variadic templating? I'm going to implement such a solution if nobody has objections. (cc: @dblaikie for thoughts)

Sat, Sep 2, 9:00 PM

Fri, Sep 1

davide committed rL312357: [TTI] Fix getGEPCost() for geps with a single operand..
[TTI] Fix getGEPCost() for geps with a single operand.
Fri, Sep 1, 12:55 PM
davide closed D37277: [TTI] Fix getGEPCost() for geps with a single operand by committing rL312357: [TTI] Fix getGEPCost() for geps with a single operand..
Fri, Sep 1, 12:55 PM
davide committed rL312353: [TTI] Initialize a value to trigger a crash deterministically..
[TTI] Initialize a value to trigger a crash deterministically.
Fri, Sep 1, 12:37 PM
davide added inline comments to D37174: NewGVN: Make sure we don't incorrectly use PredicateInfo when doing PHI of ops.
Fri, Sep 1, 12:24 PM
davide accepted D37175: Fix PR/33305. caused by trying to simplify expressions in phi of ops that should have no leaders..

I gave a round of light testing to this during the morning and it didn't expose any significant issue.
I agree with your plan, this looks like a good starting point. I'm not exactly looking forward to seeing fuzzer generated crazy miscompiles, so I hope this sticks together.

Fri, Sep 1, 12:04 PM
davide added a comment to D37355: Add CalledValuePropagation pass.

As Eli, I'd like to see other examples (I'm confident you can find some). In the meanwhile, a first review.

Fri, Sep 1, 11:54 AM
davide updated subscribers of D33987: [MergeICmps][WIP] Initial MergeICmps prototype.

Sorry for being a little late to the party, but life took over recently :(
I don't mind the structure of the pass as is (and I saw @dberlin approved which increases my confidence).
I see this was reverted because it breaks the bot. Clement, I suppose you plan to recommit this soon, and after that happens, maybe @zhendongsu can enable the new cl::opt and find new trophies for the fuzzers we have available? Zhendong, what do you think?

Fri, Sep 1, 11:27 AM

Thu, Aug 31

davide committed rL312267: [TypeSystem] Reduce code duplication merging two almost identical functions..
[TypeSystem] Reduce code duplication merging two almost identical functions.
Thu, Aug 31, 11:49 AM

Tue, Aug 29

davide added inline comments to D37277: [TTI] Fix getGEPCost() for geps with a single operand.
Tue, Aug 29, 2:38 PM
davide created D37277: [TTI] Fix getGEPCost() for geps with a single operand.
Tue, Aug 29, 2:37 PM
davide added a comment to D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.

All possibly addressed in r312015. Thanks for the comments, guys.

Tue, Aug 29, 10:26 AM
davide committed rL312015: [LoopUnroll] Make the test for PR33437 actually useful..
[LoopUnroll] Make the test for PR33437 actually useful.
Tue, Aug 29, 10:25 AM

Mon, Aug 28

davide added a comment to D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.

I added a cl::opt...

You seemed to forget to use it in the test though :)

I would also give basic blocks and variables more meaningful names (specifically, I'd rename header and preheader blocks to show clearer, what happens in the test).

Michael

Mon, Aug 28, 8:03 PM
davide accepted D36985: [LoopUnswitch] Fix a simple bug which disables loop unswitch for select statement.
Mon, Aug 28, 5:16 PM
davide accepted D37232: [InstCombine] Teach select01 helper of foldSelectIntoOp to handle vector splats.
Mon, Aug 28, 2:54 PM
davide committed rL311922: [LoopUnroll] Properly update loop structure in case of successful peeling..
[LoopUnroll] Properly update loop structure in case of successful peeling.
Mon, Aug 28, 1:30 PM
davide closed D37153: [LoopUnroll] Properly update loop structure in case of successful peeling by committing rL311922: [LoopUnroll] Properly update loop structure in case of successful peeling..
Mon, Aug 28, 1:30 PM
davide added a comment to D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.

The issue was indeed that peeling was forced only on systemz.
I added a cl::opt and I was able to reproduce the bug on my machine (x86-64 Linux), and I was able to reduce the testcase to something much smaller:

Mon, Aug 28, 1:00 PM
davide committed rL311915: [LoopUnroll] Add a cl::opt to force peeling, for testing purposes..
[LoopUnroll] Add a cl::opt to force peeling, for testing purposes.
Mon, Aug 28, 12:52 PM
davide added a comment to D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.

I think I need a new cl::opt to force peeling count from loop unrolling.

Mon, Aug 28, 12:09 PM
davide added a comment to D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.

Hi,

The change looks good to me, but I'd like to see a more simplified test. We probably don't need 90% of its instructions, and also it doesn't need to be SystemZ-specific.

Michael

Mon, Aug 28, 11:36 AM

Sat, Aug 26

davide committed rL311838: [NewGVN] Use `auto` when the type is obvious NFCI..
[NewGVN] Use `auto` when the type is obvious NFCI.
Sat, Aug 26, 3:32 PM
davide accepted D37185: [Dominators] Remove redundant explicit template instantiation..

LGTM

Sat, Aug 26, 2:07 PM

Fri, Aug 25

davide retitled D37153: [LoopUnroll] Properly update loop structure in case of successful peeling from [LoopUnroll] Properly update the loop preheader in case of successful peeling to [LoopUnroll] Properly update loop structure in case of successful peeling.
Fri, Aug 25, 5:01 PM
davide committed rL311805: [Verifier] Diagnose invalid DIType references instead of crashing..
[Verifier] Diagnose invalid DIType references instead of crashing.
Fri, Aug 25, 3:11 PM
davide committed rL311804: [Inliner] Only compute fully inline cost when remarks are enabled..
[Inliner] Only compute fully inline cost when remarks are enabled.
Fri, Aug 25, 3:02 PM
davide updated the diff for D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.

Update Trip count/tripmultiple asking them to ScalarEvolution, as suggested by Eli.

Fri, Aug 25, 1:28 PM
davide added inline comments to D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.
Fri, Aug 25, 12:45 PM
davide updated subscribers of D37059: [ELF] - LTO: do not optimize away symbols accessed from linkerscript..

FWIW, this is just an incarnation of a more general problem, i.e. the fact that the interaction between LTO and linker scripts is/wasn't well studied in the past.
I guess we may want to fix this case, but I'm afraid several other problems will arise.
I wonder when/where we should draw the line between supporting arbitrary interactions between the two or just suggesting people to pass -fno-lto to an handful of files in their link, as GCC seems to be doing.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65252
(also, cc @pcc for thoughts)

Fri, Aug 25, 10:39 AM
davide created D37153: [LoopUnroll] Properly update loop structure in case of successful peeling.
Fri, Aug 25, 10:25 AM
davide closed D37144: [gold] Fix up a new test to allow it to pass on non x86 builds..
Fri, Aug 25, 9:21 AM
davide added a comment to D37144: [gold] Fix up a new test to allow it to pass on non x86 builds..

LGTM. And yes, I concur a bot is essential for fixing catching these early.

Fri, Aug 25, 8:05 AM

Wed, Aug 23

davide accepted D36865: [Reassociate] Do not drop debug location if replacement is missing.

LGTM

Wed, Aug 23, 2:23 PM
davide added inline comments to D36311: [ThinLTO] Add GraphTraits for FunctionSummaries.
Wed, Aug 23, 6:41 AM
davide requested changes to D37059: [ELF] - LTO: do not optimize away symbols accessed from linkerscript..

Yes, I was afraid this wasn't enough.

Wed, Aug 23, 6:34 AM
davide added a comment to D37059: [ELF] - LTO: do not optimize away symbols accessed from linkerscript..

What happens if foo is specified in the linker script but actually doesn't appear in any bitcode file?
IIRC we should error in that case.

Wed, Aug 23, 6:09 AM
davide added a comment to D37059: [ELF] - LTO: do not optimize away symbols accessed from linkerscript..

Out of curiosity, where did you hit this?
Is there any way to test that the assignment updates the value of foo to be 1 ?

Wed, Aug 23, 5:43 AM
davide committed rL311541: [gold] Test we don't strip globals when producing relocatables..
[gold] Test we don't strip globals when producing relocatables.
Wed, Aug 23, 2:44 AM
davide added a reviewer for D36351: [lld][ELF] Add profile guided section layout: pcc.
Wed, Aug 23, 2:28 AM · lld
davide committed rL311540: [InstCombine] Fold branches with irrelevant conditions to a constant..
[InstCombine] Fold branches with irrelevant conditions to a constant.
Wed, Aug 23, 2:17 AM
davide closed D36975: [InstCombine] Fold branches with irrelevant conditions to a constant by committing rL311540: [InstCombine] Fold branches with irrelevant conditions to a constant..
Wed, Aug 23, 2:17 AM

Tue, Aug 22

davide added a comment to D36351: [lld][ELF] Add profile guided section layout.

Independently from the style issues, this patch as is is a no-go.
Peter tried to extend this work and hit the N^2 pretty badly, so I'm not sure it makes sense to check-in it as is.

Tue, Aug 22, 1:29 PM · lld
davide updated the diff for D36975: [InstCombine] Fold branches with irrelevant conditions to a constant.

Address David's comments.

Tue, Aug 22, 3:23 AM
davide added a comment to D36975: [InstCombine] Fold branches with irrelevant conditions to a constant.

Indeed. I changed it & added a test while I was there.

Tue, Aug 22, 3:22 AM

Mon, Aug 21

davide added a comment to D36865: [Reassociate] Do not drop debug location if replacement is missing.

I don't think we should drop DI in other cases (your reasoning is correct).

Mon, Aug 21, 12:59 PM
davide accepted D36561: [InstCombine] Move the checks for pointer types in getMaskedTypeForICmpPair earlier in the function.

This is a decent cleanup. Feel free to submit without pre-commit review when it's obvious they're correct (and they're NFC).

Mon, Aug 21, 12:51 PM
davide created D36975: [InstCombine] Fold branches with irrelevant conditions to a constant.
Mon, Aug 21, 12:01 PM
davide added a reviewer for D36957: [ELF] - Make IR symbols be visible when doing relocatable link.: pcc.
Mon, Aug 21, 10:33 AM
davide added a comment to D36957: [ELF] - Make IR symbols be visible when doing relocatable link..

Apologies, this should be fine.
Peter made me realize we should never see STB_LOCAL here.

Mon, Aug 21, 10:32 AM
davide abandoned D36959: Alternative fix for PR33097.

Oh, fair enough, my bad.
I guess George's patch should go in instead. Let me abandon this.

Mon, Aug 21, 10:30 AM
davide added a comment to D36959: Alternative fix for PR33097.

Also, FWIW, we should consider making the same change to the gold plugin.

Mon, Aug 21, 10:21 AM
davide added inline comments to D36959: Alternative fix for PR33097.
Mon, Aug 21, 10:17 AM
davide added inline comments to D36959: Alternative fix for PR33097.
Mon, Aug 21, 10:09 AM
davide updated subscribers of D36847: [Support] Add reentrant start/stop Timer methods.

Personally, I'd still be interested in understanding whether this can be fixed in the consumers.
You probably want to hear what @rsmith thinks (i.e. whether it's feasible to fix this in clang, which seems to be the only place where this code would be used).
In case you can't, I'm OK with revisiting/allowing this feature in Support/.

Mon, Aug 21, 10:07 AM
davide added a comment to D36961: [APFloat] Fix IsInteger() for DoubleAPFloat.

Thanks for your review. I added a test for the other case and committed in r311351.

Mon, Aug 21, 9:53 AM