davide (Davide Italiano)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 12 2014, 1:58 PM (157 w, 3 d)

Recent Activity

Fri, Nov 17

davide committed rL318580: [ABI] Remove dead code that was copy-pasted all around. NFCI..
[ABI] Remove dead code that was copy-pasted all around. NFCI.
Fri, Nov 17, 4:54 PM
davide committed rL318579: [Module] Throw away some more commented code. NFCI..
[Module] Throw away some more commented code. NFCI.
Fri, Nov 17, 4:52 PM
davide committed rL318577: [ABI/SysV] Remove more dead code. NFCI..
[ABI/SysV] Remove more dead code. NFCI.
Fri, Nov 17, 4:35 PM
davide committed rL318576: [Core] Garbage collect dead code untouched in years. NFCI..
[Core] Garbage collect dead code untouched in years. NFCI.
Fri, Nov 17, 4:34 PM
davide added a comment to D40177: performance improvements for ThunderX2 T99.

I'm under the impression this is not NFC.
Presumably, the changes in the scheduler reflect in a different schedule being produced.
You can then check the IR (or MIR) with the new schedule.
If you feel motivated, you can check the test before (standalone), so that the commit will highlight the differences pre/post & will make the changeset more readable.
If no change in the schedule is produced, I'm confused.

Fri, Nov 17, 4:01 PM

Thu, Nov 16

davide accepted D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple..

LGTM. Thanks Chandler.

Thu, Nov 16, 4:40 PM
davide accepted D39864: Fix for CFI type tests lowering assert. .

LGTM with Peter comment's addressed (and please don't forget to switch this to a SmallSetVector ;)

Thu, Nov 16, 2:09 PM
davide added a comment to D39864: Fix for CFI type tests lowering assert. .

(unless you don't care about the order at all, in which case you might consider adding a comment).

Thu, Nov 16, 12:41 PM
davide added inline comments to D39864: Fix for CFI type tests lowering assert. .
Thu, Nov 16, 12:38 PM

Wed, Nov 15

davide committed rL318348: [POSIX] Replace assert with llvm_unreachable(). NFCI..
[POSIX] Replace assert with llvm_unreachable(). NFCI.
Wed, Nov 15, 3:39 PM
davide added a comment to D38566: [SimplifyCFG] don't sink common insts too soon (PR34603).

Until ~ 1 year ago, SimplifyCFG was a single pass, and everything was fine (and IMHO, that's how it should be).
Then we found out it had bad interactions with inlining, so we decided to split in two passes. That's OK, as long as it's una tantum.
Keeping adding knobs to tweak the 4-5 different SimplifyCFG invocations in-tree is a slippery rope.

I'm not opposed to splitting it up, but that's a larger effort / follow-up to this.
Creating a struct of options for this pass was discussed as a reasonable way to solve PR34603, so that's what I've implemented:
https://bugs.llvm.org/show_bug.cgi?id=34603#c20

Wed, Nov 15, 12:21 PM
davide added a comment to D38566: [SimplifyCFG] don't sink common insts too soon (PR34603).

One issue with the struct approach. Suppose I opt-bisect to a failure caused by one of these simplifyCFG passes. How do I invoke that specific version of simplifycfg from opt to debug it or even produce a reduced test case.

Wed, Nov 15, 12:18 PM
davide added a comment to D38566: [SimplifyCFG] don't sink common insts too soon (PR34603).

Until ~ 1 year ago, SimplifyCFG was a single pass, and everything was fine (and IMHO, that's how it should be).
Then we found out it had bad interactions with inlining, so we decided to split in two passes. That's OK, as long as it's una tantum.
Keeping adding knobs to tweak the 4-5 different SimplifyCFG invocations in-tree is a slippery rope.

Wed, Nov 15, 11:23 AM
davide added a comment to D38566: [SimplifyCFG] don't sink common insts too soon (PR34603).

The proliferation of cl::opt(s) maybe hints SimplifyCFG is growing a little too much? & might need to be split in separate passes.
SimplifyCFG is a pretty fundamental canonicalization pass & should be almost free to run. For sinking, we have a separate pass, so maybe it's worth taking a look at whether that's fixable instead of patching SimplifyCFG behaviour? I really would like to prevent SimplifyCFG to become the next Instcombine.

Wed, Nov 15, 11:18 AM
davide closed D38512: Added phdr upper bound checks to ElfObject.
Wed, Nov 15, 9:59 AM
davide removed a reviewer for D39993: Remove dead code: davide.
Wed, Nov 15, 9:58 AM

Tue, Nov 14

davide committed rL318217: [EntryExitInstrumenter] Placate GCC, the semicolon is redundant. NFCI..
[EntryExitInstrumenter] Placate GCC, the semicolon is redundant. NFCI.
Tue, Nov 14, 3:14 PM
davide accepted D40035: [LoopRotate] processLoop should return true even if it just simplified the loop latch without making any other changes.

Other than that, lg.

Tue, Nov 14, 3:14 PM
davide added a comment to D40035: [LoopRotate] processLoop should return true even if it just simplified the loop latch without making any other changes.

I'm under the impression the fix is correct, a couple of comments inline.

Tue, Nov 14, 3:11 PM
davide added inline comments to D39982: [IRBuilder] Set the insert point and debug location together.
Tue, Nov 14, 1:18 PM · debug-info
davide accepted D39986: [LSR] Expand: Use the replaced value's debug loc (PR25630).

This one LGTM, thanks Vedant!

Tue, Nov 14, 11:28 AM
davide added inline comments to D39982: [IRBuilder] Set the insert point and debug location together.
Tue, Nov 14, 11:21 AM · debug-info

Mon, Nov 13

davide added inline comments to D39977: [llvm-objcopy] Support the rest of the ELF formats.
Mon, Nov 13, 6:13 PM
davide accepted D39993: Remove dead code.

Seems safe (assuming it doesn't break windows)

Mon, Nov 13, 5:55 PM
davide added inline comments to D39084: [PM] Port BoundsChecking to the new PM..
Mon, Nov 13, 5:32 PM
davide accepted D39085: [PM] Wire up support for the bounds checking sanitizer with the new PM..

This looks fine once the other revision goes in.

Mon, Nov 13, 3:55 PM
davide accepted D39084: [PM] Port BoundsChecking to the new PM..

LGTM modulo comments. I thought we were done with this monkey work.

Mon, Nov 13, 3:55 PM
davide added a comment to D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple..

I have few comments. As Sanjoy already approved, feel free to update this and check in without another round trip.
Most of them can be addressed in follow-ups, but I thought they're worth mentioning regardless. Thank you for pushing forward this.
It's been a long time coming :)

Mon, Nov 13, 3:48 PM

Fri, Nov 10

davide committed rL317923: [SimplifyCFG] Use auto * when the type is obvious. NFCI..
[SimplifyCFG] Use auto * when the type is obvious. NFCI.
Fri, Nov 10, 12:46 PM
davide accepted D39607: [PartialInliner] Inline vararg functions that forward varargs..

LGTM too. Thanks Florian.

Fri, Nov 10, 12:32 PM

Thu, Nov 9

davide removed a reviewer for D39802: Sched model improving on btver2: JFPU01 resource, vtestp* for xmm.: davide.
Thu, Nov 9, 12:29 AM

Wed, Nov 8

davide requested changes to D39607: [PartialInliner] Inline vararg functions that forward varargs..

Really nice :) Some comments inline to begin with.

Wed, Nov 8, 2:01 PM

Mon, Nov 6

davide committed rL317535: [Support/UNIX] posix_fallocate() can fail with EINVAL..
[Support/UNIX] posix_fallocate() can fail with EINVAL.
Mon, Nov 6, 4:47 PM
davide committed rL317527: [IPO/LowerTypesTest] Skip blockaddress(es) when replacing uses..
[IPO/LowerTypesTest] Skip blockaddress(es) when replacing uses.
Mon, Nov 6, 4:10 PM
davide closed D39695: [IPO/LowerTypesTest] Skip blockaddress when replacing uses by committing rL317527: [IPO/LowerTypesTest] Skip blockaddress(es) when replacing uses..
Mon, Nov 6, 4:10 PM
davide added inline comments to D39695: [IPO/LowerTypesTest] Skip blockaddress when replacing uses.
Mon, Nov 6, 3:57 PM
davide updated the diff for D39695: [IPO/LowerTypesTest] Skip blockaddress when replacing uses.

Address Peter's comments.

Mon, Nov 6, 3:57 PM
davide created D39695: [IPO/LowerTypesTest] Skip blockaddress when replacing uses.
Mon, Nov 6, 1:30 PM
davide added a comment to D39669: DAG: Preserve nuw when reassociating adds.

testcase?

Test is hard for this short of testing debug info, which is fragile. This prevents some regressions in a future AMDGPU commit

Mon, Nov 6, 11:18 AM
davide added a reviewer for D39669: DAG: Preserve nuw when reassociating adds: aprantl.
Mon, Nov 6, 11:17 AM
davide updated subscribers of D39689: Add a dependency from check-lldb on lld.

Not sure lld is the default? I think I always build with bfd (or gold).
I'll check later today when I'm in the office. I'm not against this change per se, but adding another dependency seems annoying. cc: @zturner

Mon, Nov 6, 9:22 AM

Sun, Nov 5

davide requested changes to D39669: DAG: Preserve nuw when reassociating adds.

testcase?

Sun, Nov 5, 5:24 PM

Fri, Nov 3

davide committed rL317393: [CallSiteSplitting] clang-format my last commit. NFCI..
[CallSiteSplitting] clang-format my last commit. NFCI.
Fri, Nov 3, 5:44 PM
davide committed rL317385: [CallSiteSplitting] Silence GCC's -Wparentheses. NFCI..
[CallSiteSplitting] Silence GCC's -Wparentheses. NFCI.
Fri, Nov 3, 4:04 PM

Thu, Nov 2

davide accepted D39581: [PartialInliner] Skip call sites where inlining fails..

LGTM with David's suggestion of moving the remark down. Thanks!

Thu, Nov 2, 6:39 PM

Wed, Nov 1

davide committed rL317145: [Core] Comparison for unsigned >= 0 is redundant. NFCI..
[Core] Comparison for unsigned >= 0 is redundant. NFCI.
Wed, Nov 1, 4:49 PM
davide committed rL317144: [XML] Simplify lambda removing unused capture. NFCI..
[XML] Simplify lambda removing unused capture. NFCI.
Wed, Nov 1, 4:48 PM
davide committed rL317143: [Interpreter] Remove unused variable usage. NFCI..
[Interpreter] Remove unused variable usage. NFCI.
Wed, Nov 1, 4:46 PM

Mon, Oct 30

davide committed rL316953: [NewGVN] Stop assuming PHI args ordering when looking at phi-of-ops..
[NewGVN] Stop assuming PHI args ordering when looking at phi-of-ops.
Mon, Oct 30, 1:20 PM
davide added a comment to D39424: [Reassociate] Remove FIXME from looptest.ll (NFC).

Please wait for Chad to comment, but this seems an OK change to me.

Mon, Oct 30, 9:54 AM
davide added a comment to D39424: [Reassociate] Remove FIXME from looptest.ll (NFC).

When was this fixed, out of curiosity?

Mon, Oct 30, 9:46 AM
davide accepted D39424: [Reassociate] Remove FIXME from looptest.ll (NFC).

LGTM

Mon, Oct 30, 9:45 AM

Fri, Oct 27

davide accepted D39381: [PartialInlineLibCalls] Teach PartialInlineLibCalls to honor nobuiltin, properly check the function signature, and check TLI::has.

Forgot to click accept

Fri, Oct 27, 2:24 PM
davide committed rL316800: [CMake] Build clang as dependency when using in-tree clang for tests..
[CMake] Build clang as dependency when using in-tree clang for tests.
Fri, Oct 27, 2:23 PM
davide added a comment to D39381: [PartialInlineLibCalls] Teach PartialInlineLibCalls to honor nobuiltin, properly check the function signature, and check TLI::has.

I guess this is fine, as this was particularly broken before, but please wait for another opinion.

Fri, Oct 27, 12:50 PM

Thu, Oct 26

davide added a comment to D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior.

As with Davide, my argument was mostly "use an existing bucket" not "it should be in bucket X". =]

I'm happy with this being under NDEBUG if this doesn't break ABI. As long as shuffle is O(N), adding an O(N) check to an O(N * log(N)) algorithm seems totally fine for NDEBUG, so I'd rather not stuff it behind the "expensive checks" macro.

Thu, Oct 26, 6:10 PM
davide added a comment to D39315: Correct the start address for exported windows functions in arm.
In D39315#908246, @sas wrote:

Same thing here, I have no idea how to go about testing something specific like this. Given that this is Windows Phone, it's even worse than simply Windows Desktop because the only way to test is by doing remote debugging. I suppose checking in binaries to the tree so we can analyze the symbols and ensure correctness is the only way to do it?

Thu, Oct 26, 11:12 AM
davide requested changes to D39315: Correct the start address for exported windows functions in arm.

This change should be unit-testable, no?

Thu, Oct 26, 10:20 AM
davide requested changes to D38037: [InstCombine] Compacting or instructions whose operands are shift instructions.
Thu, Oct 26, 10:18 AM
davide added a comment to D39314: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD.
In D39314#908115, @sas wrote:

can you please try adding a test for this one? :)

To be fully honest, I'm not 100% sure how I'm supposed to add tests for this. I looked through the directories containing unit tests and didn't find anything specific to DYLD testing.

I'm going to try to sync with @zturner to see if he is still able to run unit tests on Windows. We exclusively debug Windows remotes from a macOS or Linux host, so I don't have any setup to run local windows tests.

Thu, Oct 26, 10:04 AM
davide added a comment to D39314: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD.

can you please try adding a test for this one? :)

Thu, Oct 26, 9:38 AM
davide added a comment to D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior.

I don't really like the ever growing set of configuration macros here.

I would much prefer we have one ABI-breaking check macro and just use that for everything here...

Thanks @chandlerc. I am fine with enabling this based on LLVM_ABI_BREAKING_CHECKS. However, this macro is ON by default for +asserts builds. This means all existing +asserts bots would start failing as soon as this patch gets merged since there are about 15 unit test failures. Do we want to gate patches if they show up sort order failures uncovered by shuffling?
My idea was to control this via a separate macro so that we can isolate shuffle+sort failures into a separate bot which may make it easier to fix without gating the regular flow. Thoughts?

I agree with Chandler that we should avoid the proliferation of macros and use ABI_BREAKING_CHECKS instead.

I'm actually a bit confused by this. How is this ABI-breaking? If (1) I'm right, and it isn't ABI-breaking, and (2) we're not going to have a custom macro, then I think this should just be behind NDEBUG.

ABI_BREAKING_CHECKS is meant to be for NDEBUG-like checks that actually break the ABI. One could, in theory, build Clang with assertions, build LLVM without assertions, and still a functional compiler by overriding one of the defaults for ABI_BREAKING_CHECKS. I don't see how that applies to these checks.

Thu, Oct 26, 9:37 AM

Wed, Oct 25

davide updated subscribers of D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior.

I don't really like the ever growing set of configuration macros here.

I would much prefer we have one ABI-breaking check macro and just use that for everything here...

Thanks @chandlerc. I am fine with enabling this based on LLVM_ABI_BREAKING_CHECKS. However, this macro is ON by default for +asserts builds. This means all existing +asserts bots would start failing as soon as this patch gets merged since there are about 15 unit test failures. Do we want to gate patches if they show up sort order failures uncovered by shuffling?
My idea was to control this via a separate macro so that we can isolate shuffle+sort failures into a separate bot which may make it easier to fix without gating the regular flow. Thoughts?

Wed, Oct 25, 10:09 PM
davide updated subscribers of D39307: Fix global data symbol resolution.

Richard Smith (@rsmith) owns clang and is familiar with this.

Wed, Oct 25, 5:45 PM
davide added a comment to D39307: Fix global data symbol resolution.

Sorry, my bad, I haven't grepped properly, there are 3 internal symbols named a in libm.so.

Wed, Oct 25, 4:43 PM
davide added a comment to D39307: Fix global data symbol resolution.

Thanks for taking care of the test.

Wed, Oct 25, 4:42 PM
davide added a comment to D39307: Fix global data symbol resolution.

Thanks, I'll try this patch tomorrow.
I know this may be a little off, but how hard is to write a test for this so that it doesn't regress?

Wed, Oct 25, 4:10 PM

Tue, Oct 24

davide added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Tue, Oct 24, 9:37 PM · lld
davide added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Tue, Oct 24, 9:35 PM · lld
davide committed rL316530: [FreeBSD] Remove more dead code. NFCI..
[FreeBSD] Remove more dead code. NFCI.
Tue, Oct 24, 4:32 PM
davide committed rL316529: [ExpressionParser] Garbage-collect dead code. NFCI..
[ExpressionParser] Garbage-collect dead code. NFCI.
Tue, Oct 24, 4:29 PM
davide added a comment to D38768: Add remarks describing when a pass changes the IR instruction count of a module.

Now that there's discussion on making the new pass manager the default pass manager, I'm wondering if it'd make sense to just go ahead and implement this functionality there entirely? I was hoping to get it done with the LegacyPassManager first then port it over after.

Also, the test is pretty big now to cover all of the pass manager types. Does anyone think there's a more succinct way to do this?

Tue, Oct 24, 1:47 PM

Mon, Oct 23

davide added a comment to D39215: Default to using in-tree clang for building test executables.

I think we should just use the _COMPILER variant and default to the clang in tree (wheter possible)

Mon, Oct 23, 4:41 PM
davide accepted D39215: Default to using in-tree clang for building test executables.

I was going to propose the same :)
Maybe an heads up on lldb-dev?

Mon, Oct 23, 4:40 PM
davide committed rL316393: [lldbtests] Handle errors instead of crashing..
[lldbtests] Handle errors instead of crashing.
Mon, Oct 23, 4:18 PM
davide closed D39199: [lldbtests] Handle errors instead of crashing by committing rL316393: [lldbtests] Handle errors instead of crashing..
Mon, Oct 23, 4:18 PM
davide committed rL316390: [Symbol] Remove dead code. NFCI..
[Symbol] Remove dead code. NFCI.
Mon, Oct 23, 4:14 PM
davide added a comment to D39199: [lldbtests] Handle errors instead of crashing.

I just wanted to make sure nobody was *already* relying on that behavior. If we can get away with being strict, we should be strict.

Mon, Oct 23, 1:50 PM
davide added inline comments to D39199: [lldbtests] Handle errors instead of crashing.
Mon, Oct 23, 1:21 PM
davide added inline comments to D39145: [PM] Add pgo-memop-opt pass to the new pass manager.
Mon, Oct 23, 12:42 PM
davide accepted D39145: [PM] Add pgo-memop-opt pass to the new pass manager.

LGTM now, thanks.

Mon, Oct 23, 12:41 PM
davide added a comment to D39199: [lldbtests] Handle errors instead of crashing.

If there are cases where is_exe shouldn't be fatal, then we might consider splitting the utility into two bits (exists & is_exe).
In my mind, if you're checking whether something is executable you should run the check on a valid entity, and it's up to the caller guaranteeing the validity (we should, of course, check here).

Mon, Oct 23, 12:41 PM
davide updated the diff for D39199: [lldbtests] Handle errors instead of crashing.
Mon, Oct 23, 12:37 PM
davide added a comment to D39199: [lldbtests] Handle errors instead of crashing.

Apologies, I generally do (but I forgot this time), let me update the patch.

Mon, Oct 23, 12:36 PM
davide added inline comments to D39199: [lldbtests] Handle errors instead of crashing.
Mon, Oct 23, 12:13 PM
davide created D39199: [lldbtests] Handle errors instead of crashing.
Mon, Oct 23, 11:08 AM
davide created D39198: [lldbtests] Handle errors instead of crashing.
Mon, Oct 23, 11:06 AM
davide abandoned D39198: [lldbtests] Handle errors instead of crashing.
Mon, Oct 23, 11:06 AM
davide committed rL316355: [lldbtest] Simplify removing an unneeded else. NFCI..
[lldbtest] Simplify removing an unneeded else. NFCI.
Mon, Oct 23, 10:54 AM
davide added a comment to D38768: Add remarks describing when a pass changes the IR instruction count of a module.

It largely depends on what's your use case, but it's not unreasonable to provide this feature only for the new PM.

Mon, Oct 23, 10:26 AM
davide added a reviewer for D39181: [MemDep] DBG intrinsics don't impact abort limit for call site dependence analysis: efriedma.
Mon, Oct 23, 8:49 AM

Sun, Oct 22

davide added a comment to D39011: [SimplifyCFG] try harder to forward switch condition to phi (PR34471).

(sorry I didnt reply before but it was devmtg week)

Sun, Oct 22, 10:29 AM
davide added a comment to D39011: [SimplifyCFG] try harder to forward switch condition to phi (PR34471).

yes, that was my concern too. I think pretty much every forward data flow analysis that propagates facts can make this xform useless. If anything, it's too fragile.

Sun, Oct 22, 10:29 AM

Fri, Oct 20

davide added a comment to D39145: [PM] Add pgo-memop-opt pass to the new pass manager.

Don't we have a test for this? We should (I thought the PGO pipeline was tested, if it's not, you can probably add a test in a follow-up)

Fri, Oct 20, 3:30 PM
davide requested changes to D39145: [PM] Add pgo-memop-opt pass to the new pass manager.

Is it expected that we run this pass in the default -O1/-O2 pipeline?
I thought this was enabled only when -fprofile-instr-{generate/use} is specified.

Fri, Oct 20, 2:47 PM

Oct 19 2017

davide added a comment to D38806: DepthFirstIterator.h: Use C++11 features to call a completed method onthe set type, instead of requiring that one exists..

BTW, are there other iterators where we want to apply this trick?

Oct 19 2017, 11:05 AM
davide accepted D38806: DepthFirstIterator.h: Use C++11 features to call a completed method onthe set type, instead of requiring that one exists..

I personally don't know a better way, so I'm fine with this.

Oct 19 2017, 11:01 AM

Oct 17 2017

davide requested changes to D39038: Mark lld/test/ELF as flaky..

I really don't like this solution, in particular as a global one. Can we try to reproduce the tests that are failing?

Oct 17 2017, 10:35 PM
davide added a comment to D39011: [SimplifyCFG] try harder to forward switch condition to phi (PR34471).

What's the interaction between this code and constant propagation?

Oct 17 2017, 4:26 PM
davide added a comment to D38916: [GlobalDCE] Use DenseMap instead of unordered_multimap for GVDependencies..

When it comes to this I generally try many of them until I find something reasonable. I'm pretty sure 4 is a reasonable value, but I was wondering whether you actually ran some tests on some different values before picking "the best" (for some definition of). Unlikely it's going to matter in any way, FWIW.

Oct 17 2017, 3:17 PM