echristo (Eric Christopher)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 15 2012, 2:12 PM (274 w, 1 d)

Recent Activity

Yesterday

echristo accepted D42110: Move target MV resolver to COMDAT.

Seems reasonable.

Tue, Jan 16, 11:46 AM

Fri, Jan 12

echristo committed rL322427: Remove unused addIfPresent function..
Remove unused addIfPresent function.
Fri, Jan 12, 4:48 PM
echristo committed rC322427: Remove unused addIfPresent function..
Remove unused addIfPresent function.
Fri, Jan 12, 4:48 PM
echristo committed rLLD322426: Remove extraneous semicolon..
Remove extraneous semicolon.
Fri, Jan 12, 4:48 PM
echristo committed rL322426: Remove extraneous semicolon..
Remove extraneous semicolon.
Fri, Jan 12, 4:48 PM
echristo added a comment to D40894: [XRay][compiler-rt+llvm] Update XRay trampoline CFI and register stashing semantics.

Also the register saves strike me as a somewhat odd optimization - you could also have just thrown this into SAVE_REGISTERS I think?

Fri, Jan 12, 4:22 PM
echristo added a comment to D40894: [XRay][compiler-rt+llvm] Update XRay trampoline CFI and register stashing semantics.

A bit late, but I have some concerns here.

Fri, Jan 12, 3:33 PM
echristo accepted D41967: [PPC] Add a new register XER aliased to CARRY.

Aha. I missed something. :)

Fri, Jan 12, 2:18 PM

Thu, Jan 11

echristo added a comment to D41967: [PPC] Add a new register XER aliased to CARRY.

I think instead you want to add an alias in PPC similar to this:

Thu, Jan 11, 11:02 PM

Tue, Jan 9

echristo committed rL322133: Tidy some grammar in some comments.
Tidy some grammar in some comments
Tue, Jan 9, 3:27 PM
echristo added inline comments to D41693: [ARM][NFC] Avoid recreating MCSubtargetInfo in ARMAsmBackend.
Tue, Jan 9, 2:05 PM

Mon, Jan 8

echristo committed rL322053: Remove unused function HvxSelector::zerous..
Remove unused function HvxSelector::zerous.
Mon, Jan 8, 6:39 PM
echristo added inline comments to D41341: [X86] Disable 512-bit vectors during type legalization for prefer-vector-width.
Mon, Jan 8, 5:51 PM
echristo added a comment to D41837: Add Function multiversion to the release notes..

I think you're missing that right now it's x86 only yes? :)

Mon, Jan 8, 2:01 PM

Fri, Jan 5

echristo accepted D41737: [PowerPC] Try to move the stack pointer update instruction later in the prologue and earlier in the epilogue.

One inline comment and nit, but then I'm ok with this. Should elaborate about why we're doing this in the commit message.

Fri, Jan 5, 1:38 PM
echristo added a comment to D41737: [PowerPC] Try to move the stack pointer update instruction later in the prologue and earlier in the epilogue.

Just to verify, this is effectively hand scheduling the prologue and epilogue?

Fri, Jan 5, 1:15 PM

Thu, Jan 4

echristo added a comment to D41723: Introduce the "retpoline" x86 mitigation technique for variant #2 of the speculative execution vulnerabilities disclosed today, specifically identified by CVE-2017-5715, "Branch Target Injection", and is one of the two halves to Spectre...

Is there any plan (or willingness?) to backport this further, to 4.0.0 and even 3.4.1?

Thu, Jan 4, 11:42 AM
echristo accepted D41723: Introduce the "retpoline" x86 mitigation technique for variant #2 of the speculative execution vulnerabilities disclosed today, specifically identified by CVE-2017-5715, "Branch Target Injection", and is one of the two halves to Spectre...

LGTM of course.

Thu, Jan 4, 1:19 AM

Wed, Jan 3

echristo accepted D40819: Implement Attribute Target MultiVersioning (Improved edition!).

Couple of inline comments, otherwise I'm pretty happy. I'd wait for an ack by Richard for this though.

Wed, Jan 3, 6:32 PM

Tue, Jan 2

echristo accepted D41349: Thread MCSubtargetInfo through Target::createMCAsmBackend.

LGTM.

Tue, Jan 2, 4:55 PM

Thu, Dec 21

echristo accepted D41522: [Inliner] Restrict soft-float inlining penalty..

LGTM, thanks!

Thu, Dec 21, 5:58 PM
echristo added inline comments to D41522: [Inliner] Restrict soft-float inlining penalty..
Thu, Dec 21, 3:57 PM

Mon, Dec 18

echristo added a reviewer for D41349: Thread MCSubtargetInfo through Target::createMCAsmBackend: echristo.
Mon, Dec 18, 11:17 AM

Dec 11 2017

echristo added a comment to D41096: [X86] Initial support for prefer-vector-width function attribute.

I think one of my concerns here is that this also shows up on haswell with > 128-bit vectors. How would you want to fit that in here? A "Prefer128SIMD"?

Dec 11 2017, 4:32 PM
echristo accepted D41029: [JumpTables][PowerPC] Let targets decide which switch instructions are suitable for jump tables.

One inline comment then LGTM.

Dec 11 2017, 4:31 PM
echristo added a comment to D40893: [PowerPC] fix a bug in TCO eligibility check.

combined with r319218 "[PowerPC] Allow tail calls of fastcc functions from C CallingConv functions."

Dec 11 2017, 1:33 PM

Dec 8 2017

echristo added a comment to D40893: [PowerPC] fix a bug in TCO eligibility check.

Can you recombine this fix in with the patch that was reverted in r319218?

Dec 8 2017, 10:36 AM

Dec 7 2017

echristo added a comment to D39950: [DebugInfo] Stable sort symbols to remove non-deterministic ordering.

Well... I'm not fully convinced that the stable_sort isn't just papering over some other problem, but it seems to be producing the correct final result. If @echristo is okay with it, I'm okay with it.

Dec 7 2017, 4:50 PM · debug-info
echristo committed rL320106: Temporarily revert "[PowerPC] Allow tail calls of fastcc functions from C….
Temporarily revert "[PowerPC] Allow tail calls of fastcc functions from C…
Dec 7 2017, 2:27 PM
echristo accepted D40908: PowerPC][asan] Update asan to handle changed memory layouts in newer kernels.
Dec 7 2017, 1:44 PM
echristo accepted D40907: [PowerPC][asan] Update asan to handle changed memory layouts in newer kernels.
Dec 7 2017, 1:44 PM

Dec 6 2017

echristo added a comment to D39950: [DebugInfo] Stable sort symbols to remove non-deterministic ordering.

@echristo does it make sense to emit aranges without the symbols being emitted yet?

Dec 6 2017, 4:40 PM · debug-info
echristo added a comment to D39805: [Power9] Set MicroOpBufferSize for Power 9.

Hi Everyone,
Thank you for your comments.

In light of the feedback I'm thinking of changing this patch a little. We can set the MicroOpBufferSize = 44 since that's the actual size of the buffer.

Dec 6 2017, 3:23 PM

Dec 5 2017

echristo added inline comments to D40855: [PowerPC] LSR tunings for PowerPC.
Dec 5 2017, 2:30 PM
echristo added inline comments to D40033: [NVPTX] Initial adaptation of MCAsmStreamer/MCTargetStreamer for debug info in Cuda..
Dec 5 2017, 12:57 PM
echristo added a comment to D40033: [NVPTX] Initial adaptation of MCAsmStreamer/MCTargetStreamer for debug info in Cuda..

This needs careful review. I'll take a look as soon as I can.

Dec 5 2017, 11:44 AM

Nov 30 2017

echristo accepted D30431: [PowerPC] MachineSSA pass to reduce the number of CR-logical operations.
Nov 30 2017, 7:39 AM
echristo added a comment to D30431: [PowerPC] MachineSSA pass to reduce the number of CR-logical operations.

Some performance numbers as requested by @echristo (all run on a Power8 2GHz machine, bound to a specific physical CPU):
SPEC2006 (run time improvements - negative is good):
444.namd,868.7807,0.0000,850.4903,0.0000,-2.11%
447.dealII,630.6040,0.0000,631.2014,0.0000,0.09%
450.soplex,473.6103,0.0000,417.0427,0.0000,-11.94%
453.povray,505.4464,0.0000,487.0534,0.0000,-3.64%
401.bzip2,969.0608,0.0000,923.7337,0.0000,-4.68%
445.gobmk,868.6733,0.0000,857.6179,0.0000,-1.27%
464.h264ref,1053.8501,0.0000,1005.3811,0.0000,-4.60%
471.omnetpp,473.8660,0.0000,444.2557,0.0000,-6.25%
Changes below 1% omitted.
This was from a single run of SPEC2016 with r319300 and then same revision with this patch applied. Since it was a single run, I don't have information about the noise range, but the important thing is that the trend seems to be rather clear - performance improves with this patch.

Nov 30 2017, 7:39 AM

Nov 29 2017

echristo added a comment to D40477: Enable Partial Inlining by default.
In D40477#939609, @gyiu wrote:

This definitely needs numbers - across multiple platforms. Both performance and size of resultant binaries.

There's actually an RFC for this and I've collected numbers on PPC. Someone else collected numbers for ARM. Didn't get any replies on X86.

http://lists.llvm.org/pipermail/llvm-dev/2017-November/118752.html

Nov 29 2017, 1:24 PM
echristo added a comment to D40477: Enable Partial Inlining by default.

FWIW that's typically how we do things.

Yeah, which is why this patch seemed a bit surprising. It appears that we were previously conditionally adding it to the pipeline and had an option to turn it on/off in the pass itself. Now it's a bit of a hybrid approach and I think that warrants a comment in the pass builder:

  • It is unconditionally added to the non-LTO pipeline
  • It is conditionally added to the pre-link step in the LTO pipeline
  • The pass itself has a command line option to disable it (everywhere)
Nov 29 2017, 1:14 PM
echristo added a comment to D40477: Enable Partial Inlining by default.

This definitely needs numbers - across multiple platforms. Both performance and size of resultant binaries.

Nov 29 2017, 1:05 PM
echristo added a comment to D40477: Enable Partial Inlining by default.

FWIW that's typically how we do things.

Nov 29 2017, 1:03 PM
echristo accepted D38575: [PowerPC] Recommit r314244 with refactoring and off by default.

I'd really like to see this solved in a more generic way, but I think that's a bit far afield for a single backend so OK.

Nov 29 2017, 10:22 AM

Nov 28 2017

echristo added a comment to D40355: [PowerPC] Allow tail calls of fastcc functions from C CallingConv functions..

LGTM.

Nov 28 2017, 1:49 PM
echristo added a comment to D40497: [PowerPC] Partially enable the ISEL expansion pass..

How are we generating these isels if they're basically nops?

Nov 28 2017, 1:47 PM

Nov 27 2017

echristo accepted D40228: [Target] Make a copy of TargetOptions feature list before sorting during CodeGen.
Nov 27 2017, 5:35 PM

Nov 20 2017

echristo added a comment to D40228: [Target] Make a copy of TargetOptions feature list before sorting during CodeGen.

No strong opinion. I think I like that one more though.

Nov 20 2017, 3:40 PM
echristo accepted D40228: [Target] Make a copy of TargetOptions feature list before sorting during CodeGen.

This seems strictly more difficult to keep under control? Though I guess the assert helps.

Nov 20 2017, 3:21 PM
echristo added inline comments to D39933: Summary: Fix out-of-order stepping behavior in programs with sunk instructions.
Nov 20 2017, 2:05 PM
echristo added inline comments to D39688: [Nios2] final infrastructure addition to provide compilation of simple return from a function..
Nov 20 2017, 10:07 AM

Nov 15 2017

echristo committed rL318374: Fix thinko in last commit..
Fix thinko in last commit.
Nov 15 2017, 7:25 PM
echristo committed rL318373: Add NDEBUG checks around LLVM_DUMP_METHOD functions for Wunused-function….
Add NDEBUG checks around LLVM_DUMP_METHOD functions for Wunused-function…
Nov 15 2017, 7:18 PM
echristo committed rL318372: Need to work around the gcc Wunused-function bug as far back as gcc 6.1, update….
Need to work around the gcc Wunused-function bug as far back as gcc 6.1, update…
Nov 15 2017, 7:18 PM
echristo committed rL318371: Add NDEBUG checks around LLVM_DUMP_METHOD functions for Wunused-function….
Add NDEBUG checks around LLVM_DUMP_METHOD functions for Wunused-function…
Nov 15 2017, 7:18 PM
echristo added a reviewer for D39805: [Power9] Set MicroOpBufferSize for Power 9: atrick.

So, what's going on with the CurrCycle counter as we're going through? If we were, instead, to fix that would we still need this patch?

Nov 15 2017, 5:27 PM

Nov 14 2017

echristo added a comment to D39982: [IRBuilder] Set the insert point and debug location together.

All at once with an update tool is my preference. :)

Nov 14 2017, 1:54 PM · debug-info
echristo added a comment to D39982: [IRBuilder] Set the insert point and debug location together.

I've made a couple of comments on API naming and deprecating inline.

Nov 14 2017, 1:15 PM · debug-info

Nov 13 2017

echristo added a comment to D39081: [PM] Refactor BoundsChecking further to prepare it to be exposed both as a legacy and new PM pass..

LGTM :)

Nov 13 2017, 4:52 PM
echristo accepted D39081: [PM] Refactor BoundsChecking further to prepare it to be exposed both as a legacy and new PM pass..
Nov 13 2017, 3:43 PM
echristo accepted D39084: [PM] Port BoundsChecking to the new PM..

LGTM.

Nov 13 2017, 3:40 PM
echristo accepted D39085: [PM] Wire up support for the bounds checking sanitizer with the new PM..

LGTM.

Nov 13 2017, 3:40 PM
echristo added a comment to D32872: [PowerPC] Leverage PGO data to version/expand small/large memcpy calls.

Can you break out some of this into "NFC statistics updates" and "the rest of the patch"? It'll make it much easier to review.

Nov 13 2017, 1:45 PM
echristo accepted D39777: [PowerPC] implement mayBeEmittedAsTailCall for PPC.

Couple of comments:

Nov 13 2017, 1:42 PM

Nov 9 2017

echristo accepted D39782: [X86] Add a def file to CPU vendor, type, and subtype encodings used by Host.cpp.

Seems to be a step in the right direction anyhow.

Nov 9 2017, 10:53 PM
echristo requested changes to D39016: Add Percent Symbol In PPC Registers for Linux.

I really appreciate you doing this work. This has been something I've wanted for some time :)

Nov 9 2017, 10:50 PM

Nov 1 2017

echristo added a reverting commit for rL317152: Fix for go bindings header to match previous commit.: rL317154: Revert "Remove some of the go specific C bindings for debug info now that….
Nov 1 2017, 6:47 PM
echristo committed rL317154: Revert "Remove some of the go specific C bindings for debug info now that….
Revert "Remove some of the go specific C bindings for debug info now that…
Nov 1 2017, 6:47 PM
echristo committed rL317152: Fix for go bindings header to match previous commit..
Fix for go bindings header to match previous commit.
Nov 1 2017, 6:25 PM
echristo committed rL317151: Remove some of the go specific C bindings for debug info now that they've been….
Remove some of the go specific C bindings for debug info now that they've been…
Nov 1 2017, 6:24 PM

Oct 27 2017

echristo added a comment to D39384: Add and make use of llvm::for_each.

That looks pretty usable for someone as bad at C++ as I am so I'm in favor.

Oct 27 2017, 1:34 PM

Oct 26 2017

echristo accepted D39341: [X86][Driver] Move all of the X86 feature flags to one spot in the Options.td file and pair them up with their negations..

LGTM. Thanks.

Oct 26 2017, 1:09 PM

Oct 24 2017

echristo added a comment to D38941: [PowerPC] Use record-form instruction for Less-or-Equal -1 and Greater-or-Equal 1.

Looks much better to me. Let sfertile give a final ACK since he's been in here too, but I'm happy. One inline comment :)

Oct 24 2017, 2:26 PM

Oct 23 2017

echristo accepted D39179: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu.

This should be fine for now. Thanks!

Oct 23 2017, 11:06 AM
echristo accepted D39009: [PowerPC] Simplify a Swap if it feeds a Splat.

LGTM. Thanks!

Oct 23 2017, 9:59 AM

Oct 20 2017

echristo accepted D39143: Add a new Simulator entry for the target triple environment.

No objections here.

Oct 20 2017, 2:21 PM

Oct 18 2017

echristo accepted D38961: [PowerPC] Increase the user cost of vector instructions by their legalization cost.

I guess number of instructions is rather hard coded in. I'd definitely like to see this across other targets, but it works for now.

Oct 18 2017, 12:30 PM
echristo added inline comments to D39009: [PowerPC] Simplify a Swap if it feeds a Splat.
Oct 18 2017, 12:14 PM

Oct 17 2017

echristo added a comment to D39009: [PowerPC] Simplify a Swap if it feeds a Splat.

First pass on review.

Oct 17 2017, 10:55 AM

Oct 16 2017

echristo added inline comments to D38961: [PowerPC] Increase the user cost of vector instructions by their legalization cost.
Oct 16 2017, 1:08 PM
echristo added a comment to D38941: [PowerPC] Use record-form instruction for Less-or-Equal -1 and Greater-or-Equal 1.

Please update the comments in both the code and the testcase to express what we're looking for and why we're looking for it. Right now if something breaks no one will know why.

Oct 16 2017, 11:49 AM

Oct 13 2017

echristo accepted D31319: [PPC] Eliminate redundant sign- and zero-extensions in PPC MI Peephole pass.

LGTM.

Oct 13 2017, 6:55 PM
echristo added a reviewer for D38809: [LLVM-C] Publicly expose getters of MetadataType, TokenType: whitequark.

Adding in more reviewers. In general I'm fine here, but since they've been in this area much more I'd like to hear from them.

Oct 13 2017, 4:48 PM
echristo added a comment to D38903: [ubsan] Only use indirect RTTI in prologues on Darwin.

Given you were the last one in this code it seems reasonable to let you go for it :)

Oct 13 2017, 2:53 PM

Oct 12 2017

echristo added a comment to D38489: TargetMachine: Merge TargetMachine and LLVMTargetMachine.

Turns out this doesn't work nice with the current layering. There are some tools that just link libTarget but not libCodeGen. Merging LLVMTargetMachine and TargetMachine however forces us to move TargetMachine into libCodeGen breaking such users.

I think when merging LLVMTargetMachine and TargetMachine we also have to be consequent and merge libTarget into libCodeGen to facilitate that. Not sure how people think about that, and I'm not sure I will find the time to prepare patches for all llvm users linking to libTarget.

FWIW, I think this too would be an improvement. I find the linking of Target vs CodeGen pretty deeply confusing already. But my guess is this would indeed require a bit more work so that layers of the target which really don't require a code generator (and shouldn't) like MC bits can avoid linking with it.

Oct 12 2017, 6:27 PM

Oct 10 2017

echristo accepted D38758: Utilize DQ-Form instructions for spill/restore and fix FrameIndex elimination to only use `lis/addi` if necessary.

Code generation looks a lot more reasonable. Can you commit the change to test/CodeGen/PowerPC/sjlj.ll separately?

Oct 10 2017, 6:28 PM
echristo accepted D31878: [Asm] Add debug tracing in table-generated assembly matcher.

LGTM. Thanks!

Oct 10 2017, 6:00 PM
echristo added a comment to D31530: [ARM] Use new assembler diags for ARM.

SGTM. Thanks!

Oct 10 2017, 5:58 PM

Oct 9 2017

echristo accepted D36691: [AsmParser] Add DiagnosticString to register classes in tablegen.

Other than being relatively certain some of your lines are > 80 columns this is fine :)

Oct 9 2017, 1:51 PM
echristo added a comment to D31878: [Asm] Add debug tracing in table-generated assembly matcher.

Seems that having a value parameter for MII in all of these targets is a little less than desirable. Also, perhaps you could just add it to the base class if you're going to use it everywhere by default?

Oct 9 2017, 1:38 PM
echristo added a comment to D31530: [ARM] Use new assembler diags for ARM.

In general a fan of this FWIW:

Oct 9 2017, 1:30 PM

Oct 5 2017

echristo added a comment to D38596: Implement attribute target multiversioning.

This generally works for me modulo the things that Hal has mentioned. You'll probably want to add Richard to the review list for the Sema bits as well.

Oct 5 2017, 6:36 PM
echristo added a comment to D38190: Partial Inlining with multi-region outlining based on PGO information.

One high level comment that I added inline as well:

Oct 5 2017, 6:29 PM
echristo added a comment to D31319: [PPC] Eliminate redundant sign- and zero-extensions in PPC MI Peephole pass.

One question here: Where are the sign and zero extensions coming from? I want to make sure we're not producing extra ones where we could avoid it.

Oct 5 2017, 5:58 PM
echristo accepted D38212: [PowerPC] Add profitablilty check for conversion to mtctr loops .
Oct 5 2017, 5:42 PM
echristo accepted D38560: [PowerPC] Add missing record form instructions to the P9 Scheduling Model.

OK.

Oct 5 2017, 5:23 PM

Oct 3 2017

echristo added reviewers for D38517: Enabling new pass manager in LTO (and thinLTO) link step via -fexperimental-new-pass-manager option: chandlerc, tejohnson.
Oct 3 2017, 2:43 PM
echristo accepted D38489: TargetMachine: Merge TargetMachine and LLVMTargetMachine.

Obviously I haven't read the entire patch. I'm in favor of this change though and so will approve it :)

Oct 3 2017, 2:31 PM
echristo accepted D38204: [TargetTransformInfo] Check if function pointer is valid before calling isLoweredToCall.

LGTM. Thanks!

Oct 3 2017, 1:36 PM

Oct 2 2017

echristo accepted D27620: [Assembler] Report multiple near misses for invalid instructions.

Sorry for the delay, go ahead.

Oct 2 2017, 7:41 PM
echristo added a comment to D38482: TargetMachine: Use LLVMTargetMachine in CodeGen.

Hi Matthias,

Oct 2 2017, 7:21 PM