davide (Davide Italiano)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 12 2014, 1:58 PM (165 w, 6 d)

Recent Activity

Today

davide accepted D42182: Add LLDB_LOG_ERROR (?).

Forgot to click accept.

Wed, Jan 17, 9:01 AM
davide added a comment to D42182: Add LLDB_LOG_ERROR (?).

I think this is fine, but I'll defer to Zachary.

Wed, Jan 17, 8:56 AM

Yesterday

davide added reviewers for D34135: [JumpThreading] Use DT to avoid processing dead blocks: brzycki, kuba, sebpop.
Tue, Jan 16, 7:37 AM

Sun, Jan 14

davide committed rL322467: [BasicAA] Stop crashing when dealing with pointers > 64 bits..
[BasicAA] Stop crashing when dealing with pointers > 64 bits.
Sun, Jan 14, 5:41 PM

Fri, Jan 12

davide added a comment to D42007: [SimplifyCFG] Try to change store operation type when sinking.

I might consider holding my nose, if this restores something that was there.
I think the other two passes we have for doing sinking aren't currently enabled (GVNSink and Sink.cpp), although unless somebody puts effort in them this will always be a chicken-egg problem.
I'm not sure I'll have the time to review this patch carefully, I don't want to put you on the hook, but if you can consider an alternative, that would be great.
If you look at the original review you'll notice that I was fundamentally opposed to the change, see e.g. https://reviews.llvm.org/D38566#926530
(FWIW, it doesn't matter where we move sinking/hoisting there will be always some case that we can't get properly. We, of course should prioritize for the common case, at least IMHO).

Fri, Jan 12, 2:56 PM
davide added a comment to D42007: [SimplifyCFG] Try to change store operation type when sinking.

(As I already previously advocated), instcombine & simplifyCFG shouldn't do any hoisting/sinking whatsoever, but I only had limited energy to fight this battle.

Fri, Jan 12, 1:43 PM
davide requested changes to D42007: [SimplifyCFG] Try to change store operation type when sinking.

+1, we have a dedicated sinking pass. Not blaming on you, but unfortunately SimplifyCFG is becoming a kitchen sink, let's try to keep it at least half-sane :)

Fri, Jan 12, 1:42 PM
davide added a comment to D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF .

Very cool and close. It would be nice to function correctly without asserts, see inlined comment.

Fri, Jan 12, 11:16 AM · Restricted Project
davide removed a reviewer for D41283: [InstCombine] Missed optimization in math expression: tan(a) * cos(a) == sin(a): davide.
Fri, Jan 12, 9:57 AM
davide added a reviewer for D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF : aprantl.
Fri, Jan 12, 9:46 AM · Restricted Project
davide added a comment to D41979: [bcanalyzer] Recognize more stream types.

Have you considered checking in the final artifacts (bitcode), maybe with a short description on how to craft them in case we need to regenerate?

Fri, Jan 12, 7:43 AM
davide requested changes to D41979: [bcanalyzer] Recognize more stream types.

I don't mind the direction but this needs a test.

Fri, Jan 12, 7:28 AM

Thu, Jan 11

davide committed rL322322: [testsuite] Remove a broken test which tried to find App in bundles..
[testsuite] Remove a broken test which tried to find App in bundles.
Thu, Jan 11, 2:40 PM
davide accepted D41965: Tighten up DIFile verifier checks on checksum.

lgtm

Thu, Jan 11, 2:00 PM · debug-info
davide added reviewers for D41924: dagcombine: Transfer debug information when folding (zext (truncate x)) -> (zext (truncate x)): RKSimon, craig.topper.
Thu, Jan 11, 8:45 AM

Wed, Jan 10

davide added a comment to D41902: Remove Platform references from the Host module.

As long as:

% lldb /path/to/Foo.app
(lldb) r

Still works, then I am fine with this. The resolve executable should find the executable down inside the app bundle (like "/path/to/Foo.app/Contents/MacOS/Foo" for desktop apps and "/path/to/Foo.app/Foo" for iOS apps).

This sounds like something that would be pretty easy to write a test for with lldb-test.

Wed, Jan 10, 1:59 PM
davide committed rL322208: [XCodebuild] Catch up with recent changes (Environment.cpp)..
[XCodebuild] Catch up with recent changes (Environment.cpp).
Wed, Jan 10, 10:54 AM
davide accepted D41902: Remove Platform references from the Host module.

This LGTM but please wait for a second opinion.

Wed, Jan 10, 7:31 AM
davide accepted D41003: Silence GCC 7 warning by using an enum class..

Go ahead if you still see this warning.

Wed, Jan 10, 7:25 AM

Tue, Jan 9

davide committed rL322095: [Support] Use realpath(3) instead of trying to open a file..
[Support] Use realpath(3) instead of trying to open a file.
Tue, Jan 9, 9:28 AM
davide accepted D41871: [dotest] Remove crashinfo hook.

No objections from me. Thanks.

Tue, Jan 9, 9:18 AM

Mon, Jan 8

davide removed a reviewer for D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x): davide.
Mon, Jan 8, 3:50 PM
davide added a comment to D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x).

log(pow(x, y)) -> y*log(x)

How is this relevant? Log is only defined for positive numbers, while sin/cos/tan are valid across all numerical inputs.

Mon, Jan 8, 3:43 PM
davide resigned from D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x).

I don't feel qualified enough to say whether this can go in, somebody with fast-math experience should comment.

Mon, Jan 8, 3:43 PM
davide added a comment to D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x).

If you have a numerical explanation of why this patch can go in, I'll be happy to accept,

I'm not sure what you're looking for. Brute force sin/cos is equal to tan for every possible value?

Mon, Jan 8, 2:52 PM
davide accepted D41833: [lli] Make lli support -mcpu=native for CPU autodetection.
Mon, Jan 8, 1:12 PM
davide added a comment to D41833: [lli] Make lli support -mcpu=native for CPU autodetection.

hmm, native makes this a pain. Probably not worth the trouble.

Mon, Jan 8, 1:12 PM
davide requested changes to D41833: [lli] Make lli support -mcpu=native for CPU autodetection.

This change looks fine, but can you add a unit test (if possible)?

Mon, Jan 8, 12:49 PM
davide added inline comments to D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x).
Mon, Jan 8, 12:28 PM
davide added inline comments to D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x).
Mon, Jan 8, 11:02 AM
davide added a comment to D41793: [Debugify] Add a mode to opt to enable faster testing.

BTW: I usually do this instead of an env var to force some arguments to all invocations of a tool in lit:

llvm-lit -Dllc="llc -verify-machineinstrs"

If we can go without an enviroment variable, it's even better.
BTW, I've never heard of this flag, is it documented somewhere? If not, we probably might want to.

Probably not. I sneaked that into llvms lit.cfg a long time ago as it was only a 1-line thing but probably very few people know about it. Let me write a sentence or two for TestingGuide.rst

Turns I out I already documented it in r236462 :)

Mon, Jan 8, 9:38 AM · debug-info
davide added a comment to D41793: [Debugify] Add a mode to opt to enable faster testing.

BTW: I usually do this instead of an env var to force some arguments to all invocations of a tool in lit:

llvm-lit -Dllc="llc -verify-machineinstrs"

Mon, Jan 8, 9:21 AM · debug-info
davide added a comment to D41824: Remove use of private header in LLDB.

Opened https://bugs.llvm.org/show_bug.cgi?id=35857

Mon, Jan 8, 9:11 AM
davide updated subscribers of D41824: Remove use of private header in LLDB.
Mon, Jan 8, 9:05 AM
davide requested changes to D41824: Remove use of private header in LLDB.

I think the correct fix here is that suggested by @dblaikie. Eventually, FWIW, this header might disappear entirely and we could replace the private lldb implementation with the LLVM one (feel free to take this if you're interested).

Mon, Jan 8, 9:04 AM
davide added reviewers for D41824: Remove use of private header in LLDB: jingham, jasonmolenda, aprantl.
Mon, Jan 8, 9:02 AM
davide committed rL322006: [CVP] Replace incoming values from unreachable blocks with undef..
[CVP] Replace incoming values from unreachable blocks with undef.
Mon, Jan 8, 8:35 AM
davide closed D41812: [CVP] Replace incoming values from unreachable blocks with undef.
Mon, Jan 8, 8:35 AM
davide added a comment to D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x).

For reference. this is what GCC generates (although it's unclear whether it's a good idea to follow them)
https://godbolt.org/g/YUUKGE

Mon, Jan 8, 8:12 AM
davide requested changes to D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x).

@scanon should sign off this.

Mon, Jan 8, 8:07 AM
davide added a comment to D41496: [EarlyCSE] Salvage debug info during DCE.

@djtodoro can you commit this or you need somebody to do it on your behalf?

Mon, Jan 8, 7:51 AM
davide accepted D41496: [EarlyCSE] Salvage debug info during DCE.

LGTM modulo minor.

Mon, Jan 8, 7:50 AM

Sun, Jan 7

davide accepted D41814: [COFF] Delete CanExitEarly.

lg

Sun, Jan 7, 10:18 PM
davide committed rL321975: Revert "[SCCP] Manually fold branches on undef.".
Revert "[SCCP] Manually fold branches on undef."
Sun, Jan 7, 2:10 PM
davide committed rL321974: [SLPVectorizer] Reintroduce std::stable_sort(properlyDominates())..
[SLPVectorizer] Reintroduce std::stable_sort(properlyDominates()).
Sun, Jan 7, 2:07 PM
davide created D41812: [CVP] Replace incoming values from unreachable blocks with undef.
Sun, Jan 7, 2:03 PM
davide requested changes to D41802: Allow users of the GCOV API to extend the FileInfo class to implement custom output formats.

I don't mind this change, but before accepting, can you please show the other bits? (i.e. how do you plan to use these?)

Sun, Jan 7, 1:47 PM

Fri, Jan 5

davide accepted D41793: [Debugify] Add a mode to opt to enable faster testing.

LGTM

Fri, Jan 5, 5:02 PM · debug-info
davide requested changes to D41496: [EarlyCSE] Salvage debug info during DCE.
Fri, Jan 5, 3:27 PM
davide accepted D41787: [Utils] Simplify salvageDebugInfo, NFCI.

LGTM, thanks Vedant!

Fri, Jan 5, 3:26 PM · debug-info
davide committed rL321879: [ArchSpec] Add a unittest to complement the change in r321856..
[ArchSpec] Add a unittest to complement the change in r321856.
Fri, Jan 5, 10:02 AM
davide accepted D41626: [LiveDebugValues]Change condition for block termination recognition.

lgtm

Fri, Jan 5, 8:40 AM · debug-info
davide committed rL321873: [BasicAA] Fix linearization of shifts beyond the bitwidth..
[BasicAA] Fix linearization of shifts beyond the bitwidth.
Fri, Jan 5, 8:19 AM

Thu, Jan 4

davide committed rL321856: [ArchSpec] Don't consider Unknown MachO64 as invalid..
[ArchSpec] Don't consider Unknown MachO64 as invalid.
Thu, Jan 4, 6:51 PM
davide committed rL321833: [IRExecutionUnit] Remove broken/dead code..
[IRExecutionUnit] Remove broken/dead code.
Thu, Jan 4, 3:38 PM

Tue, Jan 2

davide accepted D41672: support phi ranges for machine-level IR.

Nice to see this for MBB, +1 modulo two minors.

Tue, Jan 2, 2:58 PM
davide accepted D41657: Do not look up symbol names when n_strx == 0.

I wonder whether you can use something like yaml2obj to craft the object? (or, assuming it's valid, llvm-mc)?
That would improve the readability a lot IMHO.
If not, can you at least add comments to the test (e.g. source file + compiler/linker version etc..) in case we need to regenerate this in the future?

llvm-objdump and llvm-nm commonly use binary tests when working with bad binaries, so this test isn't unusual. It's not clear to me how you would regenerate this mach-o with lld.

Tue, Jan 2, 2:52 PM
davide committed rL321654: [Core/Debugger] Remove some code that doesn't compile anymore..
[Core/Debugger] Remove some code that doesn't compile anymore.
Tue, Jan 2, 8:28 AM
davide committed rL321652: [MacOSX-Kernel] Remove broken KDP_IMAGEPATH support..
[MacOSX-Kernel] Remove broken KDP_IMAGEPATH support.
Tue, Jan 2, 8:25 AM
davide committed rL321651: [ARM64] Remove unused function. NFCI..
[ARM64] Remove unused function. NFCI.
Tue, Jan 2, 8:23 AM
davide added a comment to D41652: [InstCombine] Add an option to disable addrspacecast folding into GEP.

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

This is quite unfortunate. I'd like to point out I don't feel particularly comfortable to have this as a long term solution (but, maybe, OK for 6.0 & reverted in trunk).
The contract between the mid-level optimizer and the backend is that the latter should possibly accept everything produced by the former (or, FWIW, error out in some circumstances).
Maybe you can have something in your backend logic that recovers from the fact that AMDGPU doesn't know (and won't be taught) about 32-bit GEPs?
Have you considered something during legalization? [Apologies if I'm off, but I'm not really familiar with the way AMDGPU works, at least not in depth].

GEPs will actually work, but the generated assembly for loads will be worse, thus I'd like to avoid them. A better solution for LLVM 7.0 might be possible.

Tue, Jan 2, 7:35 AM
davide added a comment to D41652: [InstCombine] Add an option to disable addrspacecast folding into GEP.

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

Tue, Jan 2, 6:31 AM
davide requested changes to D41657: Do not look up symbol names when n_strx == 0.
Tue, Jan 2, 6:25 AM
davide added a comment to D41657: Do not look up symbol names when n_strx == 0.

I wonder whether you can use something like yaml2obj to craft the object? (or, assuming it's valid, llvm-mc)?
That would improve the readability a lot IMHO.
If not, can you at least add comments to the test (e.g. source file + compiler/linker version etc..) in case we need to regenerate this in the future?

Tue, Jan 2, 6:24 AM
davide accepted D41519: [BasicBlockUtils] Check for unreachable preds before updating LI in UpdateAnalysisInformation.

LGTM, thanks Anna. (sorry for the delay, I was in Europe for the end of the year).

Tue, Jan 2, 6:18 AM
davide requested changes to D41652: [InstCombine] Add an option to disable addrspacecast folding into GEP.

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?
Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

Tue, Jan 2, 5:50 AM
davide added reviewers for D41659: Implementing missing trigonometric optimizations: scanon, escha.
Tue, Jan 2, 3:23 AM
davide updated subscribers of D41659: Implementing missing trigonometric optimizations.

Something else you may want to keep in mind is that these transformations may introduce dramatic rounding errors, so they need some thoughts/analysis before they can go in (cc: @scanon), even under -ffast-math.

Tue, Jan 2, 3:22 AM
davide requested changes to D41659: Implementing missing trigonometric optimizations.

Meta point: It's unclear to me whether this is something profitable to implement.
Sure, you can probably take a textbook and implement all the possible identities written there, but, what's the point?

Tue, Jan 2, 3:21 AM

Sun, Dec 31

davide committed rL321606: [SimplifyCFG] Return to the pass manager the correct value..
[SimplifyCFG] Return to the pass manager the correct value.
Sun, Dec 31, 8:55 AM
davide committed rL321605: [Utils/Local] Use `auto` when the type is obvious. NFCI..
[Utils/Local] Use `auto` when the type is obvious. NFCI.
Sun, Dec 31, 8:52 AM
davide committed rL321604: [Utils] Remove commented debug message. NFCI..
[Utils] Remove commented debug message. NFCI.
Sun, Dec 31, 8:49 AM
davide committed rL321603: [SimplifyCFG] Stop hoisting musttail calls incorrectly..
[SimplifyCFG] Stop hoisting musttail calls incorrectly.
Sun, Dec 31, 8:48 AM

Fri, Dec 29

davide added a comment to D41418: IR: Fix BasicBlock::phis for empty blocks.

Can you add a unittest to show this issue?

Fri, Dec 29, 2:49 AM

Thu, Dec 28

davide added a comment to D41608: [InstCombine] Missed optimization in math expression: aggressive optimization with pow.

That said, this one is probably one we really want. I skimmed the code quickly and I think it's correct, but please wait for somebody else to take a look

Thu, Dec 28, 12:01 PM
davide added a comment to D41608: [InstCombine] Missed optimization in math expression: aggressive optimization with pow.

I'm slightly worried about all this bunch of missing instcombines added, as InstCombine is already really really slow.

Thu, Dec 28, 11:58 AM
davide accepted D41556: [InlineFunction] Preserve calling convention when forwarding VarArgs..

LG

Thu, Dec 28, 11:55 AM
davide accepted D41589: Avoid modifying DbgInfo while looping in salvageDebuginfo.

Yes, this sounds like classic iterator invalidation. This is probably safe.

Thu, Dec 28, 11:51 AM
davide accepted D41586: [PM] pass -debug-pass-manager flag into FunctionToLoopPassAdaptor's canonicalization PM.

LGTM

Thu, Dec 28, 11:49 AM

Tue, Dec 26

davide added a comment to D41574: [Transforms] Adding a WeakReassociate pass.

It's not entirely clear to me why you need another reassociate pass.
We should really try to share as much code as possible with the existing one instead of adding maintenance burden.

Although both of the passes perform reassociation, they are essentially very different.
The Reassociate pass aggressively modifies the topography of the expression tree, basically rewriting it, while the WeakReassociate pass uses the existing topography.
The Reassociate pass creates new instructions while the WeakReassociate pass does not.
Moreover, their method of reassociation is completely different. To modify the existing Reassociate pass to supports reassociation of the form (a_0 + a_1) + (a_2 + a_3) -> (a_0 + a_2) + (a_1 + a_3) will take a great amount of changes, changes that will almost certainly collide with existing behavior and test cases. For example, how will it decide whether (a_0 + a_1) + (a_2 + a_3) will be transformed into a_0 + (a_1 + (a_2 + a_3)) like its current behavior or to (a_0 + a_2) + (a_1 + a_3) like the new behavior?
The existing Reassociate pass lacks, by design, several kinds of reassociations, kinds that I believe can find a home in the new pass.
IMO, maintaining one big pass that has two distinguished purposes can prove to be a greater burden than maintaining two smaller passes, each dedicated to its own purpose.

Tue, Dec 26, 5:53 AM
davide added a comment to D38313: [InstCombine] Introducing Aggressive Instruction Combine pass.

@mzolotukhin may want to comment on this one before it goes in as he's spending large part of his time doing compile time work. Please wait for his opinion.

Tue, Dec 26, 3:25 AM
davide added a reviewer for D38313: [InstCombine] Introducing Aggressive Instruction Combine pass: mzolotukhin.
Tue, Dec 26, 3:25 AM
davide requested changes to D41574: [Transforms] Adding a WeakReassociate pass.

It's not entirely clear to me why you need another reassociate pass.
We should really try to share as much code as possible with the existing one instead of adding maintenance burden.

Tue, Dec 26, 3:24 AM

Sun, Dec 24

davide added a reviewer for D41563: [Transforms] Propagate TBAA info in SROA: chandlerc.
Sun, Dec 24, 4:02 AM

Sat, Dec 23

davide committed rL321402: [SCCP] Manually fold branches on undef..
[SCCP] Manually fold branches on undef.
Sat, Dec 23, 7:07 AM
davide added inline comments to D41527: [llvm-readobj] Support -needed-libs option for Mach-O files.
Sat, Dec 23, 5:40 AM
davide added a reviewer for D41467: PR35710: Nary reassociation falls into infinite loop: sanjoy.
Sat, Dec 23, 5:36 AM
davide accepted D41381: [InstSimplify] Missed optimization in math expression: squashing exp(log), log(exp).

Sorry, I clearly misread your previous diff. LGTM.

Sat, Dec 23, 5:31 AM

Fri, Dec 22

davide accepted D41527: [llvm-readobj] Support -needed-libs option for Mach-O files.

LGTM with one comment addressed. Thanks.

Fri, Dec 22, 1:06 PM
davide requested changes to D41527: [llvm-readobj] Support -needed-libs option for Mach-O files.

I'm across the pond fo the rest of the year, sorry if communication is a little slow :)

Fri, Dec 22, 12:50 PM
davide accepted D41322: [InstCombine] Missed optimization in math expression: squashing sqrt functions.
Fri, Dec 22, 11:14 AM

Thu, Dec 21

davide requested changes to D41519: [BasicBlockUtils] Check for unreachable preds before updating LI in UpdateAnalysisInformation.

I think this is on the right track, but, can you please simplify the test case so I can read it better :) ?

Thu, Dec 21, 8:34 PM
davide requested changes to D41527: [llvm-readobj] Support -needed-libs option for Mach-O files.

Can you please add a testcase, Petr?

Thu, Dec 21, 8:15 PM
davide requested changes to D41427: [SymbolFilePDB] Fix null array access when parsing the type of a function without any arguments, i.e. 'int main()' and add support to test it.
Thu, Dec 21, 6:56 AM
davide added a comment to D41427: [SymbolFilePDB] Fix null array access when parsing the type of a function without any arguments, i.e. 'int main()' and add support to test it.

Indeed, this needs to be tested.

Thu, Dec 21, 6:56 AM
davide added a comment to D41322: [InstCombine] Missed optimization in math expression: squashing sqrt functions.

LGTM.

Thu, Dec 21, 5:19 AM

Wed, Dec 20

davide requested changes to D41381: [InstSimplify] Missed optimization in math expression: squashing exp(log), log(exp).

I don't have access to my checkout here, but I'm fairly sure we do something similar in SimplifyLibCalls.
I understand many of the pattern proposed will be duplicated, which is, not ideal.
Also, I'm pretty sure @scanon once taught me (when I implemented a similar transformation in simplifylibcalls) that these can underflow/overflow pretty dramatically, so, shouldn't these only be enabled under -ffast-math ?

Wed, Dec 20, 1:32 AM

Mon, Dec 18

davide added a reviewer for D41361: [SimplifyCFG] Avoid quadratic on a predecessors number behavior in instruction sinking.: efriedma.
Mon, Dec 18, 1:56 PM

Dec 17 2017

davide added a comment to D34499: Expose __gcov_flush for parity with libgcov in the gcc project.

With that in mind:

  1. You may want to consider adding context to your patch, i.e. via git diff -U500000 &> blah.patch
  2. You may want not jump to conclusions so quickly. I'm pretty sure people care about your contributions, it's just that nobody got around to review your patch.
Dec 17 2017, 3:14 PM
davide added a reviewer for D34499: Expose __gcov_flush for parity with libgcov in the gcc project: vsk.

@ngie, Vedant can probably take a look.
Sometimes code reviews get lost, and it's generally good to ping people here or on IRC.

Dec 17 2017, 3:09 PM