echristo (Eric Christopher)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 15 2012, 2:12 PM (300 w, 3 d)

Recent Activity

Today

echristo added a comment to D49532: [DebugInfoMetadata] Added endianity field in DIBasicType to hold DW_AT_endianity attribute for DW_TAG_basic_type..

Probably squirrel it away in the generic 'flags' bucket?

Thu, Jul 19, 4:13 PM
echristo added reviewers for D49532: [DebugInfoMetadata] Added endianity field in DIBasicType to hold DW_AT_endianity attribute for DW_TAG_basic_type.: dblaikie, aprantl, dexonsmith.

I don't necessarily think this should be part of DIBasicType since it'll be used almost never. Adding other reviewers to get thoughts on location.

Thu, Jul 19, 3:49 PM
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

Getting close, one inline comment.

Thu, Jul 19, 3:45 PM
echristo accepted D49571: [x86/SLH] Clean up helper naming for return instruction handling and remove dead declaration of a call instruction handling helper..

LGTM. The comment is particularly helpful, thanks!

Thu, Jul 19, 3:21 PM

Yesterday

echristo added a comment to D46230: For x86_64, gcc 7.2 under Amazon Linux AMI sets its paths to x86_64-amazon-linux.

LGTM.

-eric

Hi Eric, I do not have commit access to trunk. Could you commit the change for me? Thanks.

Wed, Jul 18, 12:27 PM · Restricted Project
echristo accepted D46230: For x86_64, gcc 7.2 under Amazon Linux AMI sets its paths to x86_64-amazon-linux.

LGTM.

Wed, Jul 18, 11:30 AM · Restricted Project
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

  1. I can split it, no problems.
  2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

Let me try to elaborate a bit, I agree that I'm not very clear above.

I'm not a fan of the generic "non default debug options". It feels misleading. I think we probably want to separate it by "dwarf extensions", and "dwarf version".

As far as the error message itself: "debug option" sounds like an option to debug clang rather than a debug information option. Perhaps say "debug information option" rather than "debug option"?

Wed, Jul 18, 11:09 AM
echristo added a comment to D46230: For x86_64, gcc 7.2 under Amazon Linux AMI sets its paths to x86_64-amazon-linux.

It's usually preferred to submit patches with full context - it'll make this a bit easier if you do please?

Wed, Jul 18, 10:57 AM · Restricted Project
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

  1. I can split it, no problems.
  2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.
Wed, Jul 18, 10:55 AM
echristo added a comment to D49434: Put "built-in" function definitions in global Used list, for LTO. (fix bug 34169).
In D49434#1165381, @pcc wrote:

I don't think that globalopt is the right place for this because there is no guarantee that the pass is run. If we wanted to do something about this, it seems like a better place would be in lib/Analysis/ModuleSummaryAnalysis.cpp where we decide which functions are GC roots.

Also this is only a subset of the functions that the code generator can generate calls to. There is a list of runtime functions in llvm/include/llvm/CodeGen/RuntimeLibcalls.def, but as far as I know it isn't 100% comprehensive. Using that list would probably be better than just 3 functions though.

Wed, Jul 18, 9:38 AM
echristo accepted D49043: [llvm-objdump] Add -demangle (-C) option.

Concerned about why... but let's just do it for now and figure out the test problem later. It's not a problem with the code as far as I can tell.

Wed, Jul 18, 9:23 AM

Tue, Jul 17

echristo accepted D49427: [x86/SLH] Major refactoring of SLH implementaiton. There are two big changes that are intertwined here:.

LGTM. Looks like a nice set of API simplifications.

Tue, Jul 17, 3:47 PM
echristo added a comment to D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target..

I think you should break it out on an option by option basis. Just warning on "non-standard" options doesn't really make sense.

Tue, Jul 17, 3:14 PM
echristo accepted D48146: ELF: Implement --icf=safe using address-significance tables..

You'll want to wait on Rui for an actual LGTM, but this looks good to me and will match the llvm support so we can try this out in more detail and see where we should move the proposals.

Tue, Jul 17, 3:02 PM
echristo added a comment to D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

Checking out the archives for this list might come in handy:

Tue, Jul 17, 2:58 PM
echristo accepted D48143: CodeGen: Add a target option for emitting .addrsig directives for all address-significant symbols..

LGTM. Not a fan of adding more stuff to TargetOptions, but for now it'll do.

Tue, Jul 17, 2:56 PM
echristo accepted D47744: MC: Implement support for new .addrsig and .addrsig_sym directives..

LGTM. Thanks and sorry for the delay.

Tue, Jul 17, 2:54 PM
echristo added a comment to D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

Has ELFOSABI_HERMITCORE been officially allocated?

I'm not aware of any official entity managing these ELFOSABI numbers, and have neither found references to one at the Binutils and LLVM websites.

Tue, Jul 17, 2:18 PM
echristo added a comment to D49043: [llvm-objdump] Add -demangle (-C) option.

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

If you need to make minor changes to get the build bots working, no need for further review from my point of view.

I just asked @echristo if he could give this patch a try on non-x86 architecture, to be sure it was the problem ! :)

Tue, Jul 17, 2:09 PM
echristo accepted D49433: [x86/SLH] Add the design document for Speculative Load Hardening, a Spectre v1 mitigation..

LGTM. Small organizational thoughts, but that's it.

Tue, Jul 17, 1:59 PM
echristo added a comment to D48663: [Power9] Code Cleanup - Remove needsAggressiveScheduling().
Tue, Jul 17, 1:28 PM
echristo committed rC337321: Remove unnecessary trailing ; in macro intrinsic definition..
Remove unnecessary trailing ; in macro intrinsic definition.
Tue, Jul 17, 1:27 PM
echristo committed rL337321: Remove unnecessary trailing ; in macro intrinsic definition..
Remove unnecessary trailing ; in macro intrinsic definition.
Tue, Jul 17, 1:27 PM

Mon, Jul 16

echristo added a comment to D49378: [x86/SLH] Completely rework how we sink post-load hardening past data invariant instructions to be both more correct and much more powerful..

As a quick change this looks ok, some thoughts for cleanup:

Mon, Jul 16, 4:12 PM

Thu, Jul 12

echristo added inline comments to D44824: [Spectre] Introduce a new pass to do speculative load hardening to mitigate Spectre variant #1 for x86..
Thu, Jul 12, 10:17 PM
echristo accepted D44824: [Spectre] Introduce a new pass to do speculative load hardening to mitigate Spectre variant #1 for x86..

This is obviously good to start and to iterate on.

Thu, Jul 12, 10:09 PM
echristo accepted D49220: [x86] Teach the EFLAGS copy lowering to handle much more complex control flow patterns including forks, merges, and even cyles..

LGTM, a couple of inline requests. One small and one maybe less so.

Thu, Jul 12, 9:19 PM

Wed, Jul 11

echristo committed rL336885: Remove the unused m_signal member variable, but leave the code that gets it out….
Remove the unused m_signal member variable, but leave the code that gets it out…
Wed, Jul 11, 8:58 PM
echristo committed rL336884: Remove unused variable m_header as it hasn't been used since it was.
Remove unused variable m_header as it hasn't been used since it was
Wed, Jul 11, 8:57 PM
echristo committed rL336877: Temporarily revert "Recommit r328307: [IPSCCP] Use constant range information….
Temporarily revert "Recommit r328307: [IPSCCP] Use constant range information…
Wed, Jul 11, 6:58 PM
echristo accepted D49211: [x86] Fix EFLAGS copy lowering to correctly handle walking past uses in multiple successors where some of the uses end up killing the EFLAGS register..

The change looks obvious, I think the test is fine too, but MIR test cases aren't fun to read. :)

Wed, Jul 11, 5:51 PM
echristo committed rL336870: Add -allow-deprecated-dag-overlap to one of the experimental webassembly target….
Add -allow-deprecated-dag-overlap to one of the experimental webassembly target…
Wed, Jul 11, 5:06 PM

Tue, Jul 10

echristo added a comment to D48964: Add support for writing HermitCore (https://hermitcore.org) ELF binaries..

Has ELFOSABI_HERMITCORE been officially allocated?

Tue, Jul 10, 3:31 PM

Mon, Jul 9

echristo committed rC336630: Update crash diagnostics test to avoid attempting to write into various.
Update crash diagnostics test to avoid attempting to write into various
Mon, Jul 9, 6:06 PM
echristo committed rL336630: Update crash diagnostics test to avoid attempting to write into various.
Update crash diagnostics test to avoid attempting to write into various
Mon, Jul 9, 6:06 PM

Sun, Jul 1

echristo committed rL336072: Add an entry for rodata constant merge sections to the default.
Add an entry for rodata constant merge sections to the default
Sun, Jul 1, 5:21 PM

Fri, Jun 29

echristo committed rUNW336012: Revert "Revert "Support for multiarch runtimes layout"".
Revert "Revert "Support for multiarch runtimes layout""
Fri, Jun 29, 1:32 PM
echristo committed rL336012: Revert "Revert "Support for multiarch runtimes layout"".
Revert "Revert "Support for multiarch runtimes layout""
Fri, Jun 29, 1:32 PM

Thu, Jun 28

echristo added a comment to D48663: [Power9] Code Cleanup - Remove needsAggressiveScheduling().

Looking pretty good at this point. I am still curious why we've got the scheduling change here...

Thu, Jun 28, 12:35 PM

Wed, Jun 27

echristo added inline comments to D48281: [llvm-readobj] Add -hex-dump (-x) option.
Wed, Jun 27, 8:35 PM
echristo accepted D48554: Handle absolute symbols as branch targets in disassembly..

I'm ok here. Ray?

Wed, Jun 27, 2:54 PM
echristo added inline comments to D48663: [Power9] Code Cleanup - Remove needsAggressiveScheduling().
Wed, Jun 27, 2:52 PM
echristo accepted D48622: [llvm-objdump] Add -x --all-headers options.

Oh, sure :)

Wed, Jun 27, 1:50 PM
echristo added inline comments to D48663: [Power9] Code Cleanup - Remove needsAggressiveScheduling().
Wed, Jun 27, 11:21 AM

Tue, Jun 26

echristo added inline comments to D48554: Handle absolute symbols as branch targets in disassembly..
Tue, Jun 26, 2:19 PM

Mon, Jun 25

echristo committed rL335558: Add a warning if someone attempts to add extra section flags to sections.
Add a warning if someone attempts to add extra section flags to sections
Mon, Jun 25, 4:58 PM
echristo committed rL335547: Fix unsigned/signed comparison failure in unittest..
Fix unsigned/signed comparison failure in unittest.
Mon, Jun 25, 4:16 PM
echristo added a reviewer for D48554: Handle absolute symbols as branch targets in disassembly.: paulsemel.
Mon, Jun 25, 3:10 PM
echristo updated subscribers of D48100: Append new attributes to the end of an AttributeList..

I've added a couple of inline comments here - between this and the comments in the post-commit review from dlj it seems like we might want to revert this for now and figure out the best way forward.

Mon, Jun 25, 12:57 PM

Fri, Jun 22

echristo accepted D48468: [SelectionDAG] Remove debug locations from ConstantSD(FP)Nodes.

LGTM.

Fri, Jun 22, 2:46 PM

Thu, Jun 21

echristo added inline comments to D48405: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used.
Thu, Jun 21, 11:53 PM
echristo added a comment to D47211: [X86] Implement more of x86-64 large and medium PIC code models.

Sorry about the late review. I've added a lot of comment and documentation requests for the code and a couple of questions.

Thu, Jun 21, 3:17 PM
echristo added inline comments to D48405: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used.
Thu, Jun 21, 12:32 PM
echristo accepted D48448: [X86] Correct the inline assembly implementations of __movsb/w/d/q and __stosw/d/q to mark registers/memory as modified.
Thu, Jun 21, 11:48 AM
echristo added a comment to D48405: [ELF] Assign RF_EXEC rank even if --no-rosegment or SECTIONS command is used.

I think this is obsolete after the other patch?

Thu, Jun 21, 11:27 AM
echristo accepted D48406: [ELF] Make non-writable non-executable PROGBITS sections closer to .text.

OK, this ranking looks good to me. Go ahead and put your long comment with the layout into your commit message and do fix up the tests.

Thu, Jun 21, 11:17 AM
echristo committed rL335207: Add some explanatory text to the associated symbol support..
Add some explanatory text to the associated symbol support.
Thu, Jun 21, 12:20 AM
echristo committed rL335208: Remove FIXME comment about WIP..
Remove FIXME comment about WIP.
Thu, Jun 21, 12:20 AM

Tue, Jun 19

echristo updated subscribers of D46560: [SelectionDAG] Don't crash on inline assembly errors when the inline assembly return type is a struct..

Good enough for me. I hadn't had time to look. Thanks Eli.

Tue, Jun 19, 6:53 PM

Jun 19 2018

echristo added a reviewer for D46560: [SelectionDAG] Don't crash on inline assembly errors when the inline assembly return type is a struct.: bogner.
Jun 19 2018, 4:02 PM
echristo added a comment to D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups.".

As a drive by comment: While I think that consumers should be able to handle this case (obviously the bfd/gold workarounds here aren't always effective), I do think that we should probably implement this as well so that the consumers don't have to deal with it in a lot of cases.

Jun 19 2018, 3:28 PM
echristo accepted D48329: [ELF] Support -z initfirst.

Seems reasonable and fairly obvious.

Jun 19 2018, 1:48 PM

Jun 18 2018

echristo accepted D48298: [ELF] Uniquify --wrap list..

One nit, otherwise LGTM.

Jun 18 2018, 3:34 PM
echristo committed rL334990: Tidy comment language and explanation..
Tidy comment language and explanation.
Jun 18 2018, 3:26 PM
echristo committed rL334989: Pull non-lazy stub table emission into a separate function alongside.
Pull non-lazy stub table emission into a separate function alongside
Jun 18 2018, 3:25 PM
echristo committed rL334988: Add return statements to make it clear that all of these are mutually exclusive….
Add return statements to make it clear that all of these are mutually exclusive…
Jun 18 2018, 3:25 PM

Jun 14 2018

echristo accepted D47989: [llvm-readobj] Add -string-dump (-p) option.
Jun 14 2018, 1:16 PM

Jun 13 2018

echristo added a reviewer for D47959: [DAG] Avoid needing to walk out legalization tables. NFCI.: bogner.

Adding Justin as I'm more comfortable with him reviewing this than me :)

Jun 13 2018, 2:20 PM
echristo added a comment to D48090: [PPC64] global dynamic to initial exec relaxation.

Can you give me a pointer to the spec describing these relaxations?

Yep, the abi has a section describing each of the tls relaxations: http://openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655241_61284.html

Jun 13 2018, 9:19 AM

Jun 12 2018

echristo accepted D48088: [PowerPC] The __float128 type should only be available on Power9.
Jun 12 2018, 12:15 PM
echristo added a reviewer for D47989: [llvm-readobj] Add -string-dump (-p) option: dblaikie.

One inline nit and adding Dave here.

Jun 12 2018, 10:34 AM
echristo updated subscribers of D47691: [NVPTX] Ignore target-cpu and -features for inling.

The description you had is basically how and why this stuff works. Just
switch "older cpu" for "minimum gpu" and I think all of this should still
make sense?

Jun 12 2018, 10:05 AM

Jun 11 2018

echristo accepted D47507: [MC] [X86] Teach leaq _GLOBAL_OFFSET_TABLE(%rip), %r15 to use R_X86_64_GOTPC32 instead of R_X86_64_PC32.

One inline comment then OK.

Jun 11 2018, 11:10 PM
echristo added a comment to D45265: [PowerPC] Replace the Post RA List Scheduler with the Machine Scheduler.

LGTM. I think we should go ahead with this.

I agree with the additional work Eric suggested as well. I believe we already use the MachineScheduler on all recent power cores as well as the BG cores, so we might as well make it an across-the-board setting and simplify the code.

Jun 11 2018, 2:41 PM

Jun 7 2018

echristo added inline comments to D47507: [MC] [X86] Teach leaq _GLOBAL_OFFSET_TABLE(%rip), %r15 to use R_X86_64_GOTPC32 instead of R_X86_64_PC32.
Jun 7 2018, 5:45 PM
echristo accepted D47863: [Support] Add support for the OF_Delete open flag on posix platforms.

LGTM.

Jun 7 2018, 1:53 PM

Jun 6 2018

echristo accepted D46978: [builtins] Delay emutls deallocation for one round.

LGTM.

Jun 6 2018, 6:10 PM
echristo accepted D47861: [builtins] emutls cleanup: determine header size using sizeof.

LGTM.

Jun 6 2018, 6:09 PM
echristo added reviewers for D47691: [NVPTX] Ignore target-cpu and -features for inling: hfinkel, echristo.
Jun 6 2018, 5:46 PM
echristo accepted D46978: [builtins] Delay emutls deallocation for one round.

One inline comment, but otherwise looks ok. You could also split out all of the non-bionic/skip_destructor_rounds into cleanup and other patches as well.

Jun 6 2018, 3:21 PM
echristo accepted D47493: [llvm-objdump] Add -R option.

Some of the logic in the function feels awkward, but that's a cleanup for another time probably.

Jun 6 2018, 3:15 PM
echristo accepted D47839: Fix the test case that places intermediate in source directory.

Looks like a simple oops and feel free to fix anything like this without putting up a review in the future :)

Jun 6 2018, 11:49 AM
echristo added inline comments to D47493: [llvm-objdump] Add -R option.
Jun 6 2018, 1:39 AM
echristo accepted D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup.

Right now I don't have a better design without quite a bit more in depth look. That said, this gets us through a correctness issue and can be revisited in the future so let's go with it.

Jun 6 2018, 1:27 AM

May 30 2018

echristo committed rL333594: Add fopen to the list of builtins that we check and whitelist..
Add fopen to the list of builtins that we check and whitelist.
May 30 2018, 2:15 PM
echristo committed rC333594: Add fopen to the list of builtins that we check and whitelist..
Add fopen to the list of builtins that we check and whitelist.
May 30 2018, 2:15 PM

May 27 2018

echristo added reviewers for D41953: [LoopUnroll] Unroll and Jam: chandlerc, sanjoy.

It sounded like Hal wanted to review this and I don't know that any of the other people on the review line have any experience with loop passes and so probably shouldn't have been approving this. I might be wrong here, and if so I apologize, but it seems like this went in a bit early.

May 27 2018, 5:32 AM
echristo committed rL333357: Remove boolean argument from isSuitableFromBSS..
Remove boolean argument from isSuitableFromBSS.
May 27 2018, 4:43 AM
echristo committed rL333356: Cleanups for getKindForGlobal:.
Cleanups for getKindForGlobal:
May 27 2018, 4:27 AM
echristo committed rL333355: Tidy up some language and reword a few projects for clarity..
Tidy up some language and reword a few projects for clarity.
May 27 2018, 2:52 AM
echristo committed rL333354: Tidy some language in the xray documentation..
Tidy some language in the xray documentation.
May 27 2018, 2:23 AM

May 24 2018

echristo added a comment to D45265: [PowerPC] Replace the Post RA List Scheduler with the Machine Scheduler.

Relatedly it might be a good idea to remove needsAggressiveScheduling and just update the few testcases. I don't think it's worth maintaining the difference for processors that haven't been released in the last decade.

May 24 2018, 6:35 PM
echristo added a comment to D45265: [PowerPC] Replace the Post RA List Scheduler with the Machine Scheduler.

You might want to wait for Nemanja to ack as well.

May 24 2018, 6:30 PM
echristo accepted D45265: [PowerPC] Replace the Post RA List Scheduler with the Machine Scheduler.

Oh, I see now. Sure.

May 24 2018, 6:29 PM

May 23 2018

echristo committed rL333156: Add Builtins.def support for fread and fwrite to ensure that -fno-builtin-.
Add Builtins.def support for fread and fwrite to ensure that -fno-builtin-
May 23 2018, 11:14 PM
echristo committed rC333156: Add Builtins.def support for fread and fwrite to ensure that -fno-builtin-.
Add Builtins.def support for fread and fwrite to ensure that -fno-builtin-
May 23 2018, 11:14 PM
echristo committed rC333154: Migrate libcalls-fno-builtin.c test from checking optimized assembly.
Migrate libcalls-fno-builtin.c test from checking optimized assembly
May 23 2018, 11:05 PM
echristo committed rL333154: Migrate libcalls-fno-builtin.c test from checking optimized assembly.
Migrate libcalls-fno-builtin.c test from checking optimized assembly
May 23 2018, 11:05 PM
echristo added a comment to D46998: [XRay][compiler-rt] Remove reliance on C++ ABI features.

No strong opinions. I'm probably not the right person to review this :)

May 23 2018, 2:12 PM

May 21 2018

echristo added a comment to D46791: Make -gsplit-dwarf generally available.
In D46791#1107168, @pcc wrote:

There were a bunch of them but the last one was D47093 which has already landed :)

Probably all that needs to happen on this change is to replace the isLinux() check with isELF().

May 21 2018, 3:27 PM