Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

echristo (Eric Christopher)
User

Projects

User Details

User Since
Oct 15 2012, 2:12 PM (571 w, 19 h)

Recent Activity

Aug 16 2023

echristo committed rGe851ee92322a: Remove unused includes. (authored by echristo).
Remove unused includes.
Aug 16 2023, 4:47 PM · Restricted Project, Restricted Project

Aug 15 2023

echristo committed rGb7c7b1e530b7: Remove elses after return. (authored by echristo).
Remove elses after return.
Aug 15 2023, 12:11 PM · Restricted Project, Restricted Project
echristo committed rG7f7ef2f30918: Remove unused include of Compiler.h (authored by echristo).
Remove unused include of Compiler.h
Aug 15 2023, 12:10 PM · Restricted Project, Restricted Project
echristo committed rGec203c4db92f: Sink AArch64ExpandImm.h include from header file to use. (authored by echristo).
Sink AArch64ExpandImm.h include from header file to use.
Aug 15 2023, 12:10 PM · Restricted Project, Restricted Project

Feb 6 2023

echristo updated subscribers of D141836: [AArch64] Disable __muloti4 libcalls for AArch64.

Wanted to say thank you for your work on that proposal - I think it sets
the right tone and consensus is looking nice :)

Feb 6 2023, 7:47 PM · Restricted Project, Restricted Project

Jan 25 2023

echristo added a reviewer for D141836: [AArch64] Disable __muloti4 libcalls for AArch64: cjdb.

I would hypothesize that the number of platforms where compiler-rt is used to link aarch64 code is greater than the number that don't and there's work to make linking with compiler-rt (https://github.com/llvm/llvm-project/tree/main/llvm-libgcc) in order to replace uses on other platforms. In addition, while my employment meant I couldn't add this to libgcc in the past there's no reason why it couldn't be added as well. Has anyone tried?

Jan 25 2023, 11:33 AM · Restricted Project, Restricted Project

Jan 23 2023

echristo updated subscribers of D141836: [AArch64] Disable __muloti4 libcalls for AArch64.

The right move here is to check about muloti, not disable it unless the
aarch64 backend doesn't need it.

Jan 23 2023, 11:54 AM · Restricted Project, Restricted Project

Jan 18 2023

Herald added a project to D111272: [InlineCost] model calls to llvm.is.constant* more carefully: Restricted Project.

I think this makes sense if you want the second review :)

Jan 18 2023, 11:47 AM · Restricted Project, Restricted Project

Jan 11 2023

echristo accepted D141547: Fix format for `case` in .proto files.

LGTM, thanks!

Jan 11 2023, 2:54 PM · Restricted Project, Restricted Project

Jan 10 2023

echristo accepted D141191: [CMake][LoongArch] Add LoongArch to LLVM_ALL_TARGETS so it is built by default.

SGTM and congratulations.

Jan 10 2023, 10:51 AM · Restricted Project, Restricted Project

Aug 24 2022

echristo updated subscribers of D131270: MC: make section classification a bit more thorough.

Let's go ahead and revert in the meantime, can totally reapply later.

Aug 24 2022, 5:38 PM · Restricted Project, Restricted Project

Aug 17 2022

echristo accepted D131270: MC: make section classification a bit more thorough.

This works for me... adding Ray in case there's something I've missed or forgotten. I really thought this was handled before somewhere.

Aug 17 2022, 4:45 PM · Restricted Project, Restricted Project

Jul 13 2022

echristo added a comment to D127812: [AArch64] FMV support and necessary target features dependencies..

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

I don't think differentiating the uses here is a good idea. I think it would have been a GREAT idea about 10 years ago, but that ship has already sailed once GCC started using it that way however. We should be keeping the current behavior, otherwise we're going to have a horrible mix of target/target_version working inconsistently between platforms.

That largely is my concern as well. The existing behavior of target is just that -- the existing behavior. I think deviating from that existing behavior will be confusing in practice. Adding additional attributes doesn't improve that confusion because users then have to know to decide between two very similar attributes, which means they then need to educate themselves on the differences between them. If we're going to push them towards the documentation to correctly use the attribute anyway, that's basically the same situation they're in today with the confusing dual behavior of target.

We started with the ACLE to be sure the platforms and compilers will implement the same for Arm targets so make the developers life easier with a consistent behaviour on Arm platforms. Users of the attributes anyway need to aware of the architecture differences. Like -mtune is different between Arm/AArch64 and x86.

Jul 13 2022, 11:41 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Jun 3 2022

echristo added a reverting change for rGea8fb3b60196: [X86] combineConcatVectorOps - add support for concatenation VSELECT/BLENDV…: rG93cb6b9c83f1: Revert "[X86] combineConcatVectorOps - add support for concatenation….
Jun 3 2022, 12:31 PM · Restricted Project, Restricted Project
echristo committed rG93cb6b9c83f1: Revert "[X86] combineConcatVectorOps - add support for concatenation… (authored by echristo).
Revert "[X86] combineConcatVectorOps - add support for concatenation…
Jun 3 2022, 12:31 PM · Restricted Project, Restricted Project
echristo accepted D126988: Revert "[X86] combineConcatVectorOps - add support for concatenation VSELECT/BLENDV nodes".

OK for revert to green.

Jun 3 2022, 12:01 PM · Restricted Project, Restricted Project

Apr 18 2022

echristo added inline comments to D123964: CodeGen: Replace some uses of LLVMTargetMachine with TargetMachine.
Apr 18 2022, 8:56 PM · Restricted Project, Restricted Project

Mar 7 2022

echristo accepted D121173: Add JSON output option to llvm-remark-size-diff.

LGTM, one nit :)

Mar 7 2022, 4:23 PM · Restricted Project, Restricted Project

Feb 24 2022

echristo added a comment to D120455: [CommandLine] Remove `may only occur zero or one times!` error.

My biggest concern is llvm developing a gcc-esque "--params" style set of options which is what this would give. If we want these to be a support surface we should enable and test them as such. Just allowing it without that gives the entire ecosystem another set of support surface that we actually don't want people to use. Workarounds downstream should be handled by downstream as much as possible or via coordination with developers on a "hey, can we actually wait here" on options.

Feb 24 2022, 9:56 AM · Restricted Project, Restricted Project, Restricted Project
echristo added a comment to D120455: [CommandLine] Remove `may only occur zero or one times!` error.

I'd probably prefer that we don't do this since we're looking primarily at options that shouldn't be used outside of development, e.g. clang has a driver that does accept these kinds of things as does lld. Open to arguments for and against though :)

Feb 24 2022, 9:14 AM · Restricted Project, Restricted Project, Restricted Project

Feb 23 2022

echristo added a reviewer for D120455: [CommandLine] Remove `may only occur zero or one times!` error: echristo.
Feb 23 2022, 11:48 PM · Restricted Project, Restricted Project, Restricted Project

Jan 24 2022

echristo updated subscribers of D116821: [DebugInfo][InstrRef] Move instr-ref controlling flag out of TargetOptions.

It's mostly that we try very hard to not have #if 0 blocks in code please :)

Jan 24 2022, 9:49 AM · Restricted Project, Restricted Project

Jan 13 2022

echristo added a comment to D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library..
In D88827#3148315, @avl wrote:

@sbc100's response to the library division. I have a very slight preference for separate, but don't care enough about it to push for it, if others are opposed. Adding them as a reviewer in case it gets their attention.

My own preference is also to make ObjCopy to be separate library, though I also do not want to push it if others want opposite. I read this message https://reviews.llvm.org/D88827#2344900 and this https://reviews.llvm.org/D88827#2346399 and this https://reviews.llvm.org/D88827#2346567 responses as a consensus on making it to be part of Object library. If we do not have a consensus, I am open to discuss it further.

I changed my mind a bit since my early comment you linked, having gone cold on the review and now having come back to it. Here's my thinking: the object manipulation performed by the objcopy code is based on an internal Object class that has nothing to do with the object classes within the libObject library. By putting the two inside the same library, we risk confusion ("which type of Object do I need for this functionality?"). There are fundamental differences which would make reusing the libObject classes for the objcopy code less than ideal - it may not even be possible without a lot of work.

Jan 13 2022, 5:21 PM · Restricted Project

Dec 20 2021

echristo accepted D116065: Update DWARF fission extension attributes.
Dec 20 2021, 6:02 PM · debug-info, Restricted Project

Nov 22 2021

echristo added inline comments to D110549: [HIPSPV][1/4] Refactor HIP tool chain.
Nov 22 2021, 1:55 PM · Restricted Project

Nov 5 2021

echristo updated subscribers of D51650: Implement target_clones multiversioning.

I think you are these days too :) My offer was "past-past-me". I think
you're probably ok here, I did a rough scan, but getting someone like Aaron
for the attribute support would be good.

Nov 5 2021, 9:32 AM · Restricted Project, Restricted Project
echristo added a comment to D51650: Implement target_clones multiversioning.

FWIW I'm a bit rusty in this area myself, but thanks for doing this. Let's see if we can't get Aaron to continue reviewing :)

Nov 5 2021, 8:58 AM · Restricted Project, Restricted Project

Sep 23 2021

echristo added a comment to D109837: cmake: Remove config.guess.

Thanks! No need to wait on future approval with that change :)

Sep 23 2021, 10:03 AM · Restricted Project

Sep 22 2021

echristo accepted D109837: cmake: Remove config.guess.

This systems seems somewhat more fragile than config.guess? I guess I'm just not convinced that doing this versus the straightforward changes to config.guess are worth our effort? Can we get an idea of what changes you don't want to make to the local copy of config.guess?

Thanks!

The main feature that is missing from config.guess is the vendor detection (e.g. RedHat, OpenSuse, Ubuntu etc.), so the changes needed in config.guess to support this would be similar to what we have in clang now: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Distro.cpp#L97 There may be other features missing too, but this is just the one I'm interested in.

I also think that in general it would be nice to remove config.guess, because its license has caused some confusion with users and contributors.

Sep 22 2021, 6:36 PM · Restricted Project
echristo added a comment to D109837: cmake: Remove config.guess.

This systems seems somewhat more fragile than config.guess? I guess I'm just not convinced that doing this versus the straightforward changes to config.guess are worth our effort? Can we get an idea of what changes you don't want to make to the local copy of config.guess?

Sep 22 2021, 5:53 PM · Restricted Project

Sep 16 2021

echristo added a comment to D109837: cmake: Remove config.guess.

I think I'd rather not tell people to download config.guess. We can still modify the one we have, have there been serious problems doing so?

Sep 16 2021, 7:04 PM · Restricted Project

Sep 10 2021

echristo committed rG2d26a72f825c: nullptr initialize variables, spotted on msan bots. (authored by echristo).
nullptr initialize variables, spotted on msan bots.
Sep 10 2021, 6:11 PM

Sep 8 2021

echristo requested changes to D109192: Support: introduce public API annotation support.

Hi Saleem,

Sep 8 2021, 11:19 AM · Restricted Project, Restricted Project

Aug 23 2021

echristo added a comment to D106450: [InstCombine] Fold (gep (oneuse(gep Ptr, Idx0)), Idx1) -> (gep Ptr, (add Idx0, Idx1)) (PR51069).

Given that we have a test case and multiple people have reported regressions, can we revert in the meantime?

Especially if there's no obvious and clear forward fix, I'd appreciate if you could unblock us by reverting for now. Thanks!

Except that regresses other benchmarks that I was addressing with this patch - bullet etc.

Is the regression fixed by this patch more serious than the one introduced by it? Is it clear on how to fix the new regression?

I believe Bullet > “two internal *micro*benchmarks”

Aug 23 2021, 10:57 AM · Restricted Project

Jul 22 2021

echristo added a reviewer for D106624: [DWARF] Don't process .debug_info relocations for DWO Context: dblaikie.
Jul 22 2021, 9:49 PM · Restricted Project, Restricted Project

Jul 16 2021

echristo added a comment to D104080: [LLD][LLVM] CG Graph profile using relocations.

You can mention that this does increase -fprofile-use= object file sizes a bit but probably don't matter.

For instrumentation PGO users, -fprofile-generate= object files are much larger than -fprofile-use=, so -fprofile-use= slightly size increases don't matter.
For sample PGO users, this may increase the peak object file sizes a bit, but doesn't matter as your measurement for clang-13 shows.

Test Plan:
ninja check all, manual rtesting

This should be deleted when committing.

The size went up from 107KB to 322KB, aggregate of all the input sections.

How large is your clang-13? ~100MiB? Then this increase looks neligible.

Jul 16 2021, 8:31 AM · Restricted Project

Jul 8 2021

echristo committed rG5553d83adac6: Update Bazel overlay in GPUToGPURuntimeTransforms. (authored by echristo).
Update Bazel overlay in GPUToGPURuntimeTransforms.
Jul 8 2021, 9:39 PM

Jun 23 2021

echristo added inline comments to D104759: Indicate ABI in ARM ELF header flags.
Jun 23 2021, 6:23 AM · Restricted Project

Jun 17 2021

echristo accepted D90352: Introduce a Bazel build configuration.

As part of the group reviewing here's an explicit ack. Happy to do post commit review on the rest.

Jun 17 2021, 4:13 PM · Restricted Project, Restricted Project

May 26 2021

echristo added a comment to D103125: [Clang][WIP] Allow renaming of "clang".

I'm also not a fan of this change. From a project perspective the binary is clang and while people may wish to change the name for their own product teams it seems like that onus should be on them.

May 26 2021, 7:44 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

May 24 2021

echristo added a comment to D101475: Updating the `benchmark` dependency of Microbenchmarks.

Not clear why we're going with not updating in place? Or just trying to do this in multiple stages?

May 24 2021, 10:23 AM

May 21 2021

echristo added a comment to D102920: [SLP]Better detection of perfect/shuffles matches for gather nodes..

Given the iterations here I think that it needs a lot of inline documentation for cost model of what you're doing and what it's expecting here.

May 21 2021, 9:48 AM · Restricted Project

May 19 2021

echristo added a reviewer for D93838: [SCCP] Add Function Specialization pass: echristo.
May 19 2021, 10:27 AM · Restricted Project, Restricted Project

May 4 2021

echristo accepted D101867: [clang][test] Update -fc++-abi tests.
May 4 2021, 3:51 PM · Restricted Project
echristo added a comment to D101867: [clang][test] Update -fc++-abi tests.

Couple cleanups, but should be better now. Thanks!

May 4 2021, 3:42 PM · Restricted Project
echristo added inline comments to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.
May 4 2021, 3:14 PM · Restricted Project, Restricted Project

Apr 30 2021

echristo edited reviewers for D101464: [clang] Add -stdlib=libstdc++ support for webassembly, added: sbc100, jwakely; removed: expnkx, Restricted Project.

I think Sam or Jonathan would be good reviewers here.

Apr 30 2021, 11:03 AM · Restricted Project

Apr 28 2021

echristo accepted D101339: [DebugInfo] Add tests that we emit .eh_frame instead of .debug_frame.

Looks like they said with that change it was good for them and from my reading the IR is pretty good for a test. :)

Apr 28 2021, 1:09 PM · Restricted Project

Apr 27 2021

echristo updated subscribers of D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again.

Sounds good. It's a soft objection, mostly because if nothing else it puts
us back where we were subject to some latent bugs, but perhaps not as bad
as before (though I don't find having to use an assert build reassuring ;)

Apr 27 2021, 4:25 PM · Restricted Project, Restricted Project

Apr 26 2021

echristo added a comment to D99976: Allow invokable sub-classes of IntrinsicInst.

Interesting, I didn't expect that. Presumably this is because most places working with IntrinsicInst now have to perform two branches rather than one, and apparently it gets used a lot. Most likely this overestimates actual impact (assuming predicted branches), but still, better to avoid it if we can.

Apr 26 2021, 9:00 PM · Restricted Project, Restricted Project

Apr 20 2021

echristo added a comment to D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again.

Of the three people who commented on D17183, you and I are on the only ones in favor of this approach. I think @echristo and @jyknight both preferred approach 2. I'd like to get at least an agreement to disagree from one of them before approving this.

That's 50% in people, and 15/24 voting by changes that landed in clang/ since start of year ;) Don't know why 2/4 is "only".

Apr 20 2021, 3:39 PM · Restricted Project, Restricted Project
echristo added a comment to D99157: [XCore][Test] inline asm memory constraint not supported..

You should error rather than xfail if you can.

Apr 20 2021, 3:33 PM · Restricted Project
echristo added a comment to D99976: Allow invokable sub-classes of IntrinsicInst.

I guess my question is "why? what does this make easier/harder/etc?"

Apr 20 2021, 3:19 PM · Restricted Project, Restricted Project

Apr 16 2021

echristo accepted D100416: [TargetLowering] move "o" and "X" constraint handling to base class.

LGTM. Thanks.

Apr 16 2021, 11:32 AM · Restricted Project

Apr 15 2021

echristo updated subscribers of D98508: Restore lit feature object-emission.

SGTM. :)

Apr 15 2021, 4:35 PM · Restricted Project

Apr 10 2021

echristo added inline comments to D99173: Intrinsic::getName: require a Module argument.
Apr 10 2021, 10:15 AM · Restricted Project

Apr 6 2021

echristo accepted D99931: [Orc][examples] Add lit ToolSubst for LLJITWithRemoteDebugging example.
Apr 6 2021, 8:35 AM · Restricted Project

Apr 2 2021

echristo added inline comments to D99305: [docs] Document our norms around reverts.
Apr 2 2021, 11:54 AM · Restricted Project

Mar 29 2021

echristo added a comment to D17183: Make TargetInfo store an actual DataLayout instead of a string..

In general, I think it's extremely unfortunate that Clang and LLVM have different copies of the same information. It's a problem for way more than just this one situation. So, I really don't like choice 1 -- I think it's moving in the wrong direction.

The recent thread about compiler-rt libcalls vs size of "int" is another example of this sort of issue of duplicated info across the llvm/clang boundary being troublesome. Other times, the information in question is buried in the Target implementation in LLVM, and Clang doesn't depend upon targets being compiled in...usually, so it can't access it from there. E.g. the fact that Clang even has to come up with a DataLayout string on its own is an example of this problem.

So, I like option 2. I think a lot more of the information in TargetInfo ought to be shared with LLVM. I note that ARM already pushed a bunch of shared stuff into LLVM to reduce duplication, which is great. But, it had to put it into a rather odd place (llvm/include/llvm/Support/{ARMTargetParser.h,ARMAttributeParser.h,...), because of the layering and optionality concerns. That part is not so great -- putting all this target-specific info into "Support" doesn't feel like the best fit. There should be some sort of Target-specific location for this sort of stuff to live which Clang can depend on.

Mar 29 2021, 2:31 PM
echristo added inline comments to D99305: [docs] Document our norms around reverts.
Mar 29 2021, 9:25 AM · Restricted Project

Mar 23 2021

echristo added a comment to D96881: [InstCombine] Avoid Bitcast-GEP fusion for pointers directly from allocation functions.

LGTM per above discussion. It's not ideal, but I think it's the best we can do right now.

Mar 23 2021, 2:15 PM · Restricted Project

Mar 17 2021

echristo accepted D97186: [XCOFF][llvm-dwarfdump] support llvm-dwarfdump for XCOFF DWARF.

Thanks for review @echristo

a) Can you name it something other than empty? Maybe basic or something?

Done

b) Reasonable to loosen up the checks slightly? There's a lot of -NEXT in there that doesn't appear to be strictly necessary (unless you're autogenerating the checks - if so let me know).

Yeah, sort of auto-generating. But do we need to loosen up the checks? This DWARF info should be settled as the result of the tests comes from object files.

Mar 17 2021, 10:03 AM · Restricted Project

Mar 16 2021

echristo added inline comments to D97186: [XCOFF][llvm-dwarfdump] support llvm-dwarfdump for XCOFF DWARF.
Mar 16 2021, 10:07 PM · Restricted Project

Mar 15 2021

echristo requested changes to D97186: [XCOFF][llvm-dwarfdump] support llvm-dwarfdump for XCOFF DWARF.

I would prefer you use a checked in binary to test. I think it's reasonable to create a test case based on the dwarf emission support once you're convinced that's correct and go from there. For updates you can specify what the testcase is as a comment in the check. This is similar to a number of other tests FWIW :)

Mar 15 2021, 2:38 PM · Restricted Project
echristo added inline comments to D98401: [CodeGen] Fix backward copy propagation with -g.
Mar 15 2021, 1:46 PM · Restricted Project

Mar 11 2021

echristo added a comment to D98401: [CodeGen] Fix backward copy propagation with -g.

So, why these locations rather than somewhere else? (Also lint checks/testcase/etc).

Mar 11 2021, 8:58 AM · Restricted Project

Mar 4 2021

echristo added a comment to D97736: [Driver] Add a experimental option to link to LLVM libc..

In addition to the bikeshed below, I'm not a huge fan of this in general. I think we should assume that the libc we link is complete and thus it would just be named "libc.<ext>" and in a sysroot somewhere. Another option is perhaps looking at the rtlib option, but I'd want to see a bit of a writeup there so we can see what we'd be doing.

Mar 4 2021, 8:18 PM · Restricted Project

Feb 25 2021

echristo added a comment to D96203: [clang][patch] Modify sanitizer options names: renaming blacklist to blocklist.

Exclusion list? or exclude?

Feb 25 2021, 9:07 AM · Restricted Project

Feb 23 2021

echristo added a comment to D96035: [dsymutil][DWARFlinker] implement separate multi-thread processing for compile units..
In D96035#2582780, @avl wrote:

A non-deterministic behavior is probably not acceptable - it'd certainly be no good at Google if we ever wanted to use this for DWARF linking. Not sure how the Apple folks feel about it for classic dsymutil.

Yes, this would be a non-starter.

even if it IS deterministic in --num-threads=1, and is Not deterministic in multi-thread case?

Feb 23 2021, 12:05 PM · Restricted Project, Restricted Project

Feb 22 2021

echristo accepted D97242: Add more historic DWARF vendor extensions.

LGTM. Thanks!

Feb 22 2021, 7:35 PM · Restricted Project, debug-info

Feb 18 2021

echristo added a comment to D96906: [AMDGPU] gfx90a support.

The point is that nobody upstream even got a chance to chime in.

We are and will be taking care of any feedback provided in this review post-commit.

To be fair to @rampitec , it was not his desire to push this up in 1 big patch. We needed this upstreamed and no time was given to him to break it up into reasonably sized pieces. If it appears to be his doing/his intent, well, it should not. There have been a couple comments; I believe most addressed; comments will continue to be addressed.

Feb 18 2021, 4:54 PM · Restricted Project, Restricted Project

Feb 8 2021

echristo added a reviewer for D96203: [clang][patch] Modify sanitizer options names: renaming blacklist to blocklist: vitalybuka.

Thank you for working on this!

This changes the option names that include substring blacklist to blocklist.

I think this change works in some places, but in other places we say things like "blocklisted" which feels a bit awkward. I don't super love the name blocklist, but I don't super hate it either. All the alternative names I come up with aren't really great either, like UnsanitiziedEntities or IgnoredObjects. Maybe someone will have a better idea here than me, but I do think the current name is an improvement over the old name.

There is a pre-existing patch https://reviews.llvm.org/D82244 using block and allow, that's why I chose blocklist. Although going in cold I would prefer to use allow and deny, and block seems to me a little like cheating (too similar to black).

That was my concern as well -- I can easily imagine someone seeing "blocklisted" and thinking it was a typo for "blacklisted" and undoing the change to comments with an NFC change.

However I can "disagree and commit" (cf. https://en.wikipedia.org/wiki/Disagree_and_commit) with this choice of wording. I can search out the awkward Blocklisted and come up with different phrasing in the prose, but I think names in the program should use blocklist so that they can be easily connected to the option name for future maintainers of llvm. I'll check this.

I agree that the frontend option names and the internal variable names need to connect to one another, so if we pick a new term, it should be used consistently. I'd be mildly happier with a better term than blocklist, but I can live with that name too.

Feb 8 2021, 6:57 AM · Restricted Project

Feb 2 2021

echristo accepted D95380: Turn on the new pass manager by default.

Adding Tom here as release manager so they know.

Feb 2 2021, 8:42 PM · Restricted Project

Jan 27 2021

echristo added inline comments to D94992: [SLP]Merge reorder and reuse shuffles..
Jan 27 2021, 9:26 AM · Restricted Project

Jan 21 2021

echristo added inline comments to D92069: [NFC] [TargetRegisterInfo] add one use check to lookThruCopyLike..
Jan 21 2021, 5:16 PM · Restricted Project

Jan 20 2021

echristo accepted D95089: [Clang][OpenMP] Use `clang_cc1` test for `declare_target_device_only_compilation.cpp`.
Jan 20 2021, 2:09 PM · Restricted Project

Jan 19 2021

echristo committed rG22eb1cf89f38: Remove unused functions. (authored by echristo).
Remove unused functions.
Jan 19 2021, 2:45 PM
echristo updated subscribers of D94972: [SLP]Need to shrink the load vector after reordering..

Awesome. Thanks!

Jan 19 2021, 12:16 PM · Restricted Project
echristo added a comment to D94972: [SLP]Need to shrink the load vector after reordering..

Hmm?

Jan 19 2021, 12:12 PM · Restricted Project
echristo added a comment to D94972: [SLP]Need to shrink the load vector after reordering..

Hard to tell because there aren't any comments around any of the code that changed in either patch. Can you fix that in a separate change and actually document what everything is doing? :)

This is a separate patch with the fix only. :) There are no more patches required to fix the issue, just this one. Or do you mean something else?

Jan 19 2021, 10:42 AM · Restricted Project
echristo added a comment to D94972: [SLP]Need to shrink the load vector after reordering..

Hard to tell because there aren't any comments around any of the code that changed in either patch. Can you fix that in a separate change and actually document what everything is doing? :)

Jan 19 2021, 10:36 AM · Restricted Project
echristo added a comment to D94972: [SLP]Need to shrink the load vector after reordering..

I think this would be more clear as a combined patch rather than 3 separate and some reverted patches. Would it make more sense to revert the chain and put them all back together in a single review?

Jan 19 2021, 10:23 AM · Restricted Project
echristo added a comment to D94972: [SLP]Need to shrink the load vector after reordering..

Is the summary correct? The patch that this is fixing isn't reverted...

Jan 19 2021, 9:42 AM · Restricted Project

Jan 17 2021

echristo added a comment to D92069: [NFC] [TargetRegisterInfo] add one use check to lookThruCopyLike..

I'd like to avoid the bool completely if possible. Passing the register
might work as well if you wanted to do that, but I think it needs to be
clear API wise why and how you're doing computation where you are.

Jan 17 2021, 5:51 PM · Restricted Project
echristo updated subscribers of D92069: [NFC] [TargetRegisterInfo] add one use check to lookThruCopyLike..

Sorry to point this out now, but taking a bool in/out pointer is some
fairly awkward API. Can we do something else here?

Jan 17 2021, 5:18 PM · Restricted Project

Jan 15 2021

echristo added a comment to D94670: [DebugInfo][NFC] add a new DIE type to represent label + offset.

Can you provide a little more detail on the motivation here? Thanks!

Jan 15 2021, 2:25 AM · Restricted Project
echristo added a comment to D94668: [debug-info] [NFC] add is-a(isa<>) support for MCStreamer.

I'm not quite sure I understand. What's the issue between xcoff and emission here? Does it not support subtraction? It's been a while and I can't recall.

Jan 15 2021, 2:24 AM · Restricted Project

Jan 13 2021

echristo added a comment to D93967: [SLP]Need shrink the load vector after reordering..

If you changed the existing patch it won't be clear that this is something that needs review. Can you make a new review with the bug fix?

Jan 13 2021, 2:16 PM · Restricted Project

Jan 5 2021

echristo added a comment to D91050: [NFC] Add the EmitTargetCodeForConstantPool hook for target to customize it with MachineConstantPoolValue.

Ping...

Jan 5 2021, 4:27 PM · Restricted Project

Dec 4 2020

echristo accepted D92599: Fix for Bug 48055..
Dec 4 2020, 10:34 AM · Restricted Project
echristo added inline comments to D92599: Fix for Bug 48055..
Dec 4 2020, 10:16 AM · Restricted Project

Dec 1 2020

echristo accepted D63852: [Clang] Move assembler into a separate file.

Thanks for the explanation, lgtm.

Dec 1 2020, 12:36 PM · Restricted Project
echristo updated subscribers of D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

I think Florian's diagnosis is worth looking into for sure - and probably
is the right solution. For the regressions I had they could largely be
explained by the branch predictor on a micro level, I didn't see some of
the large scale LTO level regressions in mine.

Dec 1 2020, 11:13 AM · Restricted Project

Nov 20 2020

echristo added a comment to D70947: Add a default address space for globals to DataLayout.

Why is this useful rather than having targets set the address space used in the backend explicitly? i.e. how is what amdgpu was doing up to this point not ok? It's not really clear here.

Nov 20 2020, 11:52 AM · Restricted Project

Nov 19 2020

echristo added a reverting change for rGf7eac51b9b3f: [CostModel] remove cost-kind predicate for intrinsics in basic TTI…: rG32dd5870ee31: Temporarily Revert "[CostModel] remove cost-kind predicate for intrinsics in….
Nov 19 2020, 10:38 PM
echristo added a reverting change for rG618d555e8d92: [LoopUnroll] add test for full unroll that is sensitive to cost-model; NFC: rG32dd5870ee31: Temporarily Revert "[CostModel] remove cost-kind predicate for intrinsics in….
Nov 19 2020, 10:38 PM
echristo added a reverting change for rGdf09f825995b: [CostModel] add tests for math library calls; NFC: rG32dd5870ee31: Temporarily Revert "[CostModel] remove cost-kind predicate for intrinsics in….
Nov 19 2020, 10:38 PM
echristo added a reverting change for rG8ec7ea3ddce7: [CostModel] make default size cost for libcalls small (again): rG32dd5870ee31: Temporarily Revert "[CostModel] remove cost-kind predicate for intrinsics in….
Nov 19 2020, 10:38 PM
echristo added a reverting change for D90554: [CostModel] remove cost-kind predicate for intrinsics in basic TTI implementation: rG32dd5870ee31: Temporarily Revert "[CostModel] remove cost-kind predicate for intrinsics in….
Nov 19 2020, 10:38 PM · Restricted Project
echristo committed rG32dd5870ee31: Temporarily Revert "[CostModel] remove cost-kind predicate for intrinsics in… (authored by echristo).
Temporarily Revert "[CostModel] remove cost-kind predicate for intrinsics in…
Nov 19 2020, 10:38 PM