Page MenuHomePhabricator

echristo (Eric Christopher)
User

Projects

User Details

User Since
Oct 15 2012, 2:12 PM (430 w, 5 d)

Recent Activity

Yesterday

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!

Fri, Jan 15, 2:25 AM · Restricted Project
echristo added a comment to D94668: [debug-info] [NFC] add 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.

Fri, Jan 15, 2:24 AM · Restricted Project

Wed, Jan 13

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?

Wed, Jan 13, 2:16 PM · Restricted Project

Tue, Jan 5

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

Ping...

Tue, Jan 5, 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

Nov 16 2020

echristo accepted D91579: [ELF] --gc-sections: collect unused .gcc_except_table in section groups and associated text sections.

I think this is reasonable, if neither George or Peter say anything I think this looks ok to me.

Nov 16 2020, 6:25 PM · Restricted Project

Nov 13 2020

echristo updated subscribers of D91446: [AlwaysInliner] Call mergeAttributesForInlining after inlining.

Also probably a good idea :)

Nov 13 2020, 11:28 AM · Restricted Project
echristo accepted D91446: [AlwaysInliner] Call mergeAttributesForInlining after inlining.

I'd just seen that too. Thanks!

Nov 13 2020, 11:12 AM · Restricted Project

Nov 7 2020

echristo added a comment to D90761: [docs] Adding a Support Policy.

This looks fine to me for now. I really think it's too wordy and too elaborate and could be about 1/10th as long.

Nov 7 2020, 4:37 PM · Restricted Project

Nov 5 2020

echristo added a comment to D90856: [MLIR] Fix missing namespaces in `mlir/unittests/TableGen/OpBuildGen.cpp`.

For the future you can just commit this sort of thing. Though I admit to being curious about the lint error :)

Nov 5 2020, 8:56 AM · Restricted Project

Oct 29 2020

echristo added a comment to D89158: [NewPM] Provide method to run all pipeline callbacks, used for -O0.

Looking at BackendUtil.cpp in Clang as well as the Rust code, I'm back to thinking that we should provide a way to to all callbacks. Then in the case of passes added via TargetMachine, we should extend TargetMachine::registerPassBuilderCallbacks to also take a PassBuilder::OptimizationLevel. Then targets can skip adding optional optimization passes at -O0. Does that make sense? It would allow us to clean up Clang and Rust.

In fact I see in AMDGPUTargetMachine::adjustPassManager a check for an optimization level, so some targets are already doing this, although it looks like it's more for codegen and not IR passes.

Oct 29 2020, 6:59 PM · Restricted Project, Restricted Project
echristo accepted D89184: Support complex target features combinations.

Let's go ahead and unblock you, but getting a lot of this refactored would be great if you can. I think it's hitting the limits of the original design. :)

Oct 29 2020, 6:58 PM · Restricted Project

Oct 28 2020

echristo added a comment to D89184: Support complex target features combinations.

I'll take a look tomorrow, sorry for the delay.

Oct 28 2020, 5:47 PM · Restricted Project
echristo accepted D89995: Make the post-commit review expectations more explicit with respect to revert.

I'm going to accept this as it codifies a lot of existing intent behind how the community works. This was also fairly universally accepted 4 years ago in the original discussion and nothing has changed.

Oct 28 2020, 2:52 PM · Restricted Project

Oct 24 2020

echristo updated subscribers of D88741: [SystemZ/z/OS] Add utility class for char set conversion..

Seems reasonable to me.

Oct 24 2020, 7:23 PM · Restricted Project

Oct 21 2020

echristo added a comment to D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library..

I really do think this needs to be under object. It can be separate in there as lib/Object/Copy if you want, but I don't think it should be a parallel directory. Hopefully the updates for the move aren't overly onerous here.

Oct 21 2020, 9:09 AM · Restricted Project

Oct 14 2020

echristo accepted D89085: [llvm] Update default cutoff threshold for machine function splitter..

Awesome. Thanks for the update :)

Oct 14 2020, 10:43 AM · Restricted Project
echristo accepted D88997: Set the default for -bbsections-cold-text-prefix to .text.split..
Oct 14 2020, 10:43 AM · Restricted Project

Oct 13 2020

echristo added a comment to D89085: [llvm] Update default cutoff threshold for machine function splitter..

Sorry, didn't see this last time. Couple of inline comments, mostly nits.

Oct 13 2020, 10:07 PM · Restricted Project
echristo requested changes to D89184: Support complex target features combinations.

Mark as requesting changes :)

Oct 13 2020, 1:45 PM · Restricted Project
echristo added a comment to D89184: Support complex target features combinations.

Hi Peng,

Oct 13 2020, 1:45 PM · Restricted Project

Oct 12 2020

echristo updated subscribers of D88625: MCExpr::evaluateAsRelocatableImpl : allow evaluation of non-VK_None MCSymbolRefExpr when MCAsmLayout is available.

Oh, you're waiting on me. Uh, I'll get to this tomorrow. :)

Oct 12 2020, 9:59 PM · Restricted Project

Oct 9 2020

echristo added a comment to D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library..

I like the direction this is going, I'll take more of a deep look soon, but wanted to ask: "should this be in Object rather than a separate library?" When I'd originally asked for this to be split into its own library I'd thought that it would get added into libobject.

Oct 9 2020, 9:05 AM · Restricted Project

Oct 8 2020

echristo updated subscribers of D89085: [llvm] Update default cutoff threshold for machine function splitter..

Arguably since this is going to be very cpu dependent perhaps it should be
part of TTI?

Oct 8 2020, 5:51 PM · Restricted Project

Oct 2 2020

echristo added a comment to D88741: [SystemZ/z/OS] Add utility class for char set conversion..

Drive by as it's years since I've dealt with any of this:

Oct 2 2020, 10:00 AM · Restricted Project

Sep 18 2020

echristo added a comment to rG549e55b3d563: Temporarily Revert "[clangd] Add Random Forest runtime for code completion.".

:)

Sep 18 2020, 7:16 PM
echristo added a comment to rG549e55b3d563: Temporarily Revert "[clangd] Add Random Forest runtime for code completion.".

Which header do you think is missing? It built fine on several machines over here.

Sep 18 2020, 6:22 PM
echristo added a reverting change for rGc8757ff3aa7d: RegAllocFast: Rewrite and improve: rGdbd53a1f0c93: Temporarily Revert "RegAllocFast: Rewrite and improve".
Sep 18 2020, 6:11 PM
echristo committed rGdbd53a1f0c93: Temporarily Revert "RegAllocFast: Rewrite and improve" (authored by echristo).
Temporarily Revert "RegAllocFast: Rewrite and improve"
Sep 18 2020, 6:11 PM
echristo added a reverting change for rG9b6765e784b3: [clangd] Add Random Forest runtime for code completion.: rG549e55b3d563: Temporarily Revert "[clangd] Add Random Forest runtime for code completion.".
Sep 18 2020, 2:50 PM
echristo committed rG549e55b3d563: Temporarily Revert "[clangd] Add Random Forest runtime for code completion." (authored by echristo).
Temporarily Revert "[clangd] Add Random Forest runtime for code completion."
Sep 18 2020, 2:50 PM
echristo added a reverting change for D83814: [clangd] Add Random Forest runtime for code completion.: rG549e55b3d563: Temporarily Revert "[clangd] Add Random Forest runtime for code completion.".
Sep 18 2020, 2:50 PM · Restricted Project
echristo added a reverting change for rG455ca0ebb692: [SLP] Allow reordering of vectorization trees with reused instructions.: rGecfd8161bf43: Temporarily Revert "[SLP] Allow reordering of vectorization trees with reused….
Sep 18 2020, 12:50 PM
echristo committed rGecfd8161bf43: Temporarily Revert "[SLP] Allow reordering of vectorization trees with reused… (authored by echristo).
Temporarily Revert "[SLP] Allow reordering of vectorization trees with reused…
Sep 18 2020, 12:50 PM
echristo added a reverting change for D45263: [SLP] Allow reordering of vectorization trees with reused instructions.: rGecfd8161bf43: Temporarily Revert "[SLP] Allow reordering of vectorization trees with reused….
Sep 18 2020, 12:50 PM · Restricted Project

Sep 17 2020

echristo accepted D86485: [test] Fix FullUnroll.ll.
In D86485#2277714, @rnk wrote:

Here is the sequence of what happened as I understand it:

  • D71687 is intended to fix the full loop unrolling pragma with the NPM at -O1. The accompanying clang test covers this.
  • D71687 also adds an LLVM IR test. It is an integration test: it started from unoptimized IR and ran the full -O1 NPM pipeline. The IR happens to contain optnone. Maybe that was unintentional, since one used to be able to run clang foo.c -emit-llvm -o - | opt -O1 -S and get optimized IR. Now clang applies the optnone attribute, and that no longer works.
  • D85578 made the IR test more targeted: now it only runs the full unrolling pass instead of the O1 pipeline.

There is value in integration tests, so maybe the thing to do is to bring back the IR integration test in addition to this unit test. The unit test seems correct to me. If optnone is present, I would not expect the loop unroll metadata to be honored.

Sep 17 2020, 11:09 AM · Restricted Project

Sep 16 2020

echristo committed rGc14032281980: Use zu rather than llu format specifier for size_t (-Wformat warning fix). (authored by echristo).
Use zu rather than llu format specifier for size_t (-Wformat warning fix).
Sep 16 2020, 7:28 PM

Sep 2 2020

echristo updated subscribers of D86485: [test] Fix FullUnroll.ll.

I don't think your statement matches what I was seeing. I may have
constructed the testcase poorly, but there were differences being shown.
Also keep in mind that your clang run is testing what I've already fixed. :)

Sep 2 2020, 9:46 AM · Restricted Project

Sep 1 2020

echristo accepted D86839: [CMake] Remove -Wl,-allow-shlib-undefined which was added in rL221530.

I think this is the right thing to do here. In addition, we should not cherry pick this to any release branch so we have the opportunity to get it more fully evaluated by distros/vendors/etc over the next months.

Sep 1 2020, 7:47 PM · Restricted Project

Aug 29 2020

echristo accepted D86846: [NFC] Adding pythonenv* to .gitignore.
Aug 29 2020, 9:37 PM · Restricted Project

Aug 28 2020

echristo accepted D86505: [RFC][Target] Add a new triple called Triple::csky.
Aug 28 2020, 9:27 AM · Restricted Project

Aug 26 2020

echristo committed rGea7b1c79f73d: Add cmake test support for LLJITWithThinLTOSummaries to make sure (authored by echristo).
Add cmake test support for LLJITWithThinLTOSummaries to make sure
Aug 26 2020, 11:47 AM

Aug 24 2020

echristo added a reverting change for rG589ce5f7050d: [DebugInfo] Move constructor homing case in shouldOmitDefinition.: rG05777ab94106: Temporarily Revert "[DebugInfo] Move constructor homing case in….
Aug 24 2020, 9:54 PM
echristo committed rG05777ab94106: Temporarily Revert "[DebugInfo] Move constructor homing case in… (authored by echristo).
Temporarily Revert "[DebugInfo] Move constructor homing case in…
Aug 24 2020, 9:54 PM
echristo added a reverting change for D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.: rG05777ab94106: Temporarily Revert "[DebugInfo] Move constructor homing case in….
Aug 24 2020, 9:54 PM · Restricted Project
echristo committed rG1a2a34a38a7d: Add NDEBUG checks around debug only loop to avoid unused variable (authored by echristo).
Add NDEBUG checks around debug only loop to avoid unused variable
Aug 24 2020, 5:59 PM
echristo added a comment to D86485: [test] Fix FullUnroll.ll.

The code has a pragma to turn on loop unrolling. I actually did want to check with optnone.

Aug 24 2020, 2:24 PM · Restricted Project

Aug 23 2020

echristo accepted D86387: Fix frame pointer layout on AArch64 Linux..

LGTM from maintainability etc. Haven't really checked more so might want to wait for others.

Aug 23 2020, 12:35 PM · Restricted Project

Aug 21 2020

echristo added inline comments to D86387: Fix frame pointer layout on AArch64 Linux..
Aug 21 2020, 4:45 PM · Restricted Project
echristo added a comment to D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

I would have been using old, the default. I think we still see some performance/codesize problems with switch to new.

That might be the pattern then, just another old-pm vs new-pm difference for the switch to track.
But i'm not seeing ab umbrella bug for performance regressions on https://bugs.llvm.org/show_bug.cgi?id=46649?

One more follow up here:

One set of benchmarks we're seeing this in are also in the llvm test harness:

llvm_multisource_tsvc.TSVC-ControlFlow-flt and llvm_multisource_misc.Ptrdist-yacr2

in an FDO mode (so we have some decent branch hints for the code).

You should be able to duplicate at least some of the slowdown there.

That sadly doesn't tell me much.
Can you please provide the reproduction steps, and ideally the IR with/without this change?

It should be as straightforward as -O3 -fexperimental-new-pass-manager, if that isn't then we'll look a bit more. Those tests are in the llvm test-suite so they should be easy to get as part of your checkout.


Uhm, for N=9, i'm afraid i'm not seeing anything horrible like that:

$ /repositories/llvm-test-suite/utils/compare.py --lhs-name old-newpm --rhs-name new-newpm llvm-test-suite-old-NewPM/res-*.json vs llvm-test-suite-new-NewPM/res-*.json --merge-average --filter-hash
Tests: 41
Metric: exec_time

Program                                        old-newpm new-newpm diff 
 test-suite...marks/Ptrdist/yacr2/yacr2.test     0.77      0.74    -4.0%
 test-suite...s/Ptrdist/anagram/anagram.test     0.76      0.78     3.0%
 test-suite...flt/InductionVariable-flt.test     2.90      2.95     1.9%
 test-suite...lFlow-flt/ControlFlow-flt.test     3.48      3.44    -1.4%
 test-suite...ing-dbl/NodeSplitting-dbl.test     3.09      3.13     1.1%
 test-suite...dbl/InductionVariable-dbl.test     4.02      4.05     1.0%
 test-suite...pansion-dbl/Expansion-dbl.test     2.49      2.51     0.7%
 test-suite...pansion-flt/Expansion-flt.test     1.56      1.57     0.5%
 test-suite...lt/CrossingThresholds-flt.test     2.86      2.87     0.5%
 test-suite...lFlow-dbl/ControlFlow-dbl.test     4.05      4.03    -0.5%
 test-suite...ing-flt/Equivalencing-flt.test     1.13      1.14     0.3%
 test-suite...ow-dbl/GlobalDataFlow-dbl.test     3.20      3.21     0.3%
 test-suite.../Benchmarks/Ptrdist/bc/bc.test     0.40      0.40     0.3%
 test-suite...C/Packing-flt/Packing-flt.test     2.90      2.91     0.3%
 test-suite.../Benchmarks/Ptrdist/ft/ft.test     1.05      1.05     0.3%
 Geomean difference                                                 0.1%
       old-newpm  new-newpm       diff
count  41.000000  41.000000  41.000000
mean   2.761491   2.765264   0.001355 
std    1.382334   1.383351   0.009172 
min    0.400356   0.401544  -0.040124 
25%    2.158678   2.161544  -0.000219 
50%    2.836133   2.839078   0.000974 
75%    3.204467   3.214600   0.002932 
max    6.865056   6.881911   0.029819 

@echristo or does that match what you were seeing for test-suite? is the Ptrdist/anagram regression the one?

Aug 21 2020, 8:41 AM · Restricted Project
echristo added a comment to D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

I would have been using old, the default. I think we still see some performance/codesize problems with switch to new.

Aug 21 2020, 8:29 AM · Restricted Project
echristo added a comment to D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

I think you're still in scientific computing yes?

Embedded computing actually. Low power Arm devices, but I'm always interested in all uses of Arm devices. I have been working in the vectorizer a lot lately, and a number of the benchmarks we usually run are DSP style codes, so quite similar to HPC. But the improvements I saw included CPU's without any vectorization. Codesize was also flat in all, which I took as a good sign that the patch wasn't bad for general codegen.

Aug 21 2020, 8:29 AM · Restricted Project

Aug 20 2020

echristo added a comment to D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

We saw performance improvements from this patch. They were a little bit up-and-down, but definitely an improvement overall.

Aug 20 2020, 7:56 AM · Restricted Project
echristo added a comment to D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

One more follow up here:

One set of benchmarks we're seeing this in are also in the llvm test harness:

llvm_multisource_tsvc.TSVC-ControlFlow-flt and llvm_multisource_misc.Ptrdist-yacr2

in an FDO mode (so we have some decent branch hints for the code).

You should be able to duplicate at least some of the slowdown there.

That sadly doesn't tell me much.
Can you please provide the reproduction steps, and ideally the IR with/without this change?

Aug 20 2020, 7:53 AM · Restricted Project

Aug 19 2020

echristo added a comment to D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

One more follow up here:

Aug 19 2020, 4:12 PM · Restricted Project
echristo added a comment to D86045: [llvm-dwarfdump] --statistics: switch to json::OStream. NFC.

I feel pretty OK approving this.

(Tradeoffs I'd contemplated and discarded was renaming OS rather than OS->J everywhere. I ended up liking the readability of J more. JS might be nice as well?)

-eric

Do you mean removing the OS->J rename? That would be a bit ambiguous because people usually use OS to refer to raw_ostream and its derivatives. The JSON stream is special.

Aug 19 2020, 4:07 PM · Restricted Project
echristo added a comment to D84108: [SimplifyCFG][LoopRotate] SimplifyCFG: disable common instruction hoisting by default, enable late in pipeline.

So, we're seeing several significant (20-30%) regressions due to this in various different library benchmarks. Usually around things that are compression/decompression loops, but also other places.

Aug 19 2020, 4:02 PM · Restricted Project
echristo accepted D86045: [llvm-dwarfdump] --statistics: switch to json::OStream. NFC.

I feel pretty OK approving this.

Aug 19 2020, 2:48 PM · Restricted Project

Aug 18 2020

echristo updated subscribers of D85384: [X86] Add basic support for -mtune command line option in clang.

This would be because at that point the default cpu was that and it
probably had something to do with fallbacks.

Aug 18 2020, 10:17 PM · Restricted Project
echristo accepted D86132: [clang][driver]Add quotation mark in test/fortran.f95 to avoid false positive.
Aug 18 2020, 12:56 PM · Restricted Project

Aug 17 2020

echristo committed rGc6464c819e66: Fix another Wsign-comparison warning. (authored by echristo).
Fix another Wsign-comparison warning.
Aug 17 2020, 2:33 PM
echristo committed rG1bf0732443ee: Fix Wsign-compare warnings in test. (authored by echristo).
Fix Wsign-compare warnings in test.
Aug 17 2020, 2:00 PM

Aug 11 2020

echristo added a comment to D84068: AMDGPU/clang: Search resource directory for device libraries.

Hi All,

Aug 11 2020, 10:56 AM
echristo committed rG8155cb27a232: Fold Opcode into assert uses to fix an unused variable warning without asserts. (authored by echristo).
Fold Opcode into assert uses to fix an unused variable warning without asserts.
Aug 11 2020, 9:31 AM

Aug 10 2020

echristo committed rG9564821144f8: Test requires a debug build to pass. (authored by echristo).
Test requires a debug build to pass.
Aug 10 2020, 11:58 PM
echristo updated subscribers of D84334: [flang] Version information in flang/f18.

I'm also not sure this is passing?

Aug 10 2020, 11:17 PM · Restricted Project, Restricted Project

Aug 9 2020

echristo committed rGb529c5270c99: Add override to fix -Winconsistent-missing-override warning. (authored by echristo).
Add override to fix -Winconsistent-missing-override warning.
Aug 9 2020, 10:22 PM

Aug 6 2020

echristo updated subscribers of D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe.

I think that we should revert the original patch for now since we have a
known miscompile and a chain of fixes are required to fix it.

Aug 6 2020, 4:35 PM · Restricted Project
echristo updated subscribers of D84825: [ELF] Change tombstone values to (.debug_ranges/.debug_loc) 1 and (other .debug_*) 0.

*thumbs up on ok for release*

Aug 6 2020, 3:33 PM · Restricted Project
echristo accepted D84825: [ELF] Change tombstone values to (.debug_ranges/.debug_loc) 1 and (other .debug_*) 0.

This looks good to me. Thanks for seeing this through Ray!

Aug 6 2020, 3:21 PM · Restricted Project

Aug 5 2020

echristo added a comment to D84825: [ELF] Change tombstone values to (.debug_ranges/.debug_loc) 1 and (other .debug_*) 0.

At which point we are at an impasse and the default behavior applies. Please revert completely back to the original behavior immediately.

Thanks.

With respect I think the "request for changes" blocked the change and I am not inclined to agree we would otherwise be at an impasse without the "request for changes".
I would likely get an "Accept Revision" or a just textual "LGTM", even if there is a conditional request that ".debug_* should use 0 as well, like GNU ld". I have mentioned that my position has been weakened. (I made the reply after 5 days, not my usual style as you can tell, because the related threads have caused so much stress/pain to me. I would otherwise not be able to focus on real work. In some sense, I feel that I am fighting a really meaningless battle, even the original proposers have not said something on this patch - Why do I still care about the resolution so much? The all things make me feel I can't really make a fair decision on this patch. I regret that I was involved in the original implementation. I would definitely better if I were just a reviewer)

I sent the patch instead of reverting 3 or 4 intertwined dependent patches because I think reverting will just make the situation more confusing. I have experienced the review processes of all relevant patches and have read all the context,
so I very responsibly state again that "a simple revert of 3 or 4 dependent patches will just make the situation more confusing".

I will very appreciate feedback from other people, not my colleague, especially LLD contributors such as @psmith @grimar @jhenderson, whether we really need a flag in this patch or can we simply defer it to another thread.

Aug 5 2020, 11:38 AM · Restricted Project

Aug 4 2020

echristo added a comment to D84825: [ELF] Change tombstone values to (.debug_ranges/.debug_loc) 1 and (other .debug_*) 0.

Please implement a clear flag for

  • BFD semantics
  • New (-1/-2) semantics

And default to the BFD semantics for now.

The change in semantics is not ready to ship by default - there are lots of DWARF consumers out there and lots of people don't update them in lock-step with their compiler.

I don't know what the request is. The patch implemented:

  • .debug_ranges & .debug_loc: -2
  • .debug_*: 0 (GNU ld semantics)

If you meant full BFD semantics:

  • .debug_ranges: 1
  • .debug_*: 0

Yes, that's what I meant. (though probably applying the debug_ranges workaround to debug_loc as well would be suitable but not necessary, I think)

Did you mean

- * .debug_ranges & .debug_loc: -2
+ * .debug_ranges & .debug_loc: 1

?
I think this has gone too far a step. We really need more evidence that 1 can make DWARF consumers happier.

That's fairly easy - 0, 0 terminates a location list the same way it terminates a range list - llvm-dwarfdump, for instance, then tries to parse the following expression as the start of a new location list and errors. binutils objdump only dumps location lists that are referenced from debug_info, printint out "holes" when the location list terminates early.

eg:

$ g++-tot loc.cpp -g -ffunction-sections -Wl,-gc-sections -O3 -fuse-ld=bfd
$ llvm-dwarfdump-tot a.out
.debug_loc contents:
0x00000000: 
            (0x0040102000040402, 0x0040102000000000): 
            (0x10209f3300020000, 0x1023000000000040): <decoding error> 00 00 00 00 02 00 37 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 04 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 33 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
            (0x000000009f370002, 0x0000000000000000): error: unexpected end of data at offset 0x78 while reading [0x76, 0x7e)
$ objdump -g a.out | grep -A50 debug_loc
objdump: Warning: Hole and overlap detection requires adjacent view lists and loclists.
objdump: Warning: Contents of the .debug_loc section (loaded from a.out):
There are 40 unused bytes at the end of section .debug_loc

    Offset   Begin            End              Expression

    00000000 v000000000000002 v000000000000004 location view pair
    00000002 v000000000000004 v000000000000000 location view pair

    00000004 v000000000000002 v000000000000004 views at 00000000 for:
             0000000000401020 0000000000401020 (DW_OP_lit3; DW_OP_stack_value)
    00000018 v000000000000004 v000000000000000 views at 00000002 for:
             0000000000401020 0000000000401023 (DW_OP_lit7; DW_OP_stack_value)
    0000002c <End of list>

    0000003c v000000000000002 v000000000000004 location view pair
    0000003e v000000000000004 v000000000000000 location view pair

    00000040 <End of list>

(have to use GCC to produce the input here, since Clang's current use of base address specifiers in debug_loc doesn't trip over the issue - that doesn't diminish the problem, though)

Can you share loc.cpp?

Oh, right, sorry:

__attribute__((noinline)) void f1() { }
void f2() {
  int i = 3;
  f1();
  i = 7;
  f1();
}
int main() {
  int i = 3;
  f1();
  i = 7;
  f1();
}

That's fairly easy - 0, 0 terminates a location list the same way it terminates a range list - llvm-dwarfdump, for instance, then tries to parse the following expression as the start of a new location list and errors. binutils objdump only dumps location lists that are referenced from debug_info, printint out "holes" when the location list terminates early.

So how is BFD's choice for .debug_loc (1) better than -2? Additional warning objdump: Warning: Hole and overlap detection requires adjacent view lists and loclists. objdump: Warning: There are 40 unused bytes at the end of section .debug_loc and unneeded fffffffffffffffe ranges cannot be filtered out?

Aug 4 2020, 11:12 PM · Restricted Project
echristo added a comment to D83273: [X86] Remove the feature dependency handling in X86TargetInfo::setFeatureEnabledImpl to a table based lookup in X86TargetParser.cpp.

It's a pretty nasty revert in our downstream tree where we have prototyped future ISAs. So I'd like a little time to take a look.

@nickdesaulniers what cpu and fetaures are on your command lines. getImpliedDisabledFeatures should only be called if some feature is explicitly being disabled.

Likely, as the kernel is very explicit to disable the use of all FP extensions, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/Makefile#n57. Maybe we could just set fewer flags, but there's still probably room for improvement here, too.

Aug 4 2020, 4:26 PM · Restricted Project, Restricted Project
echristo updated subscribers of D83273: [X86] Remove the feature dependency handling in X86TargetInfo::setFeatureEnabledImpl to a table based lookup in X86TargetParser.cpp.

Ow. Can revert and reapply after we fix the caching problem perhaps?

Aug 4 2020, 3:49 PM · Restricted Project, Restricted Project
echristo updated subscribers of D85231: Protect against filenames with no extension at all..

Could maybe add an assert along with the patch as well as an assert only
test?

Aug 4 2020, 2:00 PM · Restricted Project

Jul 28 2020

echristo added a comment to D84793: [X86] Add assembler support for {disp8} and {disp32} to control the size of displacement used for memory operands..

I think I'd find the conditionals easier to "read" if they were positive rather than negative? It'd help if the comments spelled out the conditions a little more as well.

Jul 28 2020, 8:47 PM · Restricted Project

Jul 27 2020

D18036: Enabling multiple letters and digits in constraints now requires changes to proceed.

This could probably use some tests. If no current upstream port uses the feature though it makes it hard to accept. :(

Jul 27 2020, 10:07 PM
D26344: Small Data SectionKind and PPC-EABI SDA Relocation now requires changes to proceed.
Jul 27 2020, 10:05 PM
echristo accepted D84709: [X86] Add support for {disp32} to control size of jmp and jcc instructions in the assembler.
Jul 27 2020, 6:14 PM · Restricted Project

Jul 26 2020

echristo requested changes to D84519: [llvm-objdump][AMDGPU] Detect CPU string.

I'd really like all of the cpu specific bits here to not reside in the objdump binary. Ideally it should be in include/ and lib/ somewhere.

Jul 26 2020, 10:42 PM · Restricted Project

Jul 25 2020

echristo committed rG4b14ef33e81c: Temporarily Revert "Unify the return value of GetByteSize to an llvm… (authored by echristo).
Temporarily Revert "Unify the return value of GetByteSize to an llvm…
Jul 25 2020, 6:43 PM
echristo added a reverting change for rG1d9b860fb6a8: Unify the return value of GetByteSize to an llvm::Optional<uint64_t> (NFC-ish): rG4b14ef33e81c: Temporarily Revert "Unify the return value of GetByteSize to an llvm….
Jul 25 2020, 6:43 PM
echristo committed rG18975762c197: Fold StatepointBB into checks as it's only used from an NDEBUG or ASSERT… (authored by echristo).
Fold StatepointBB into checks as it's only used from an NDEBUG or ASSERT…
Jul 25 2020, 6:37 PM

Jul 23 2020

echristo committed rG3ac828b8f7a8: Use llvm::size rather than an empty loop to get the number of top level loops. (authored by echristo).
Use llvm::size rather than an empty loop to get the number of top level loops.
Jul 23 2020, 3:00 PM
echristo committed rG3a75466f41be: Temporarily Revert "Reland [lldb] Unify type name matching in… (authored by echristo).
Temporarily Revert "Reland [lldb] Unify type name matching in…
Jul 23 2020, 12:47 AM
echristo added a reverting change for rG074b121642b2: Reland [lldb] Unify type name matching in FormattersContainer: rG3a75466f41be: Temporarily Revert "Reland [lldb] Unify type name matching in….
Jul 23 2020, 12:47 AM

Jul 17 2020

echristo committed rG020545d386cf: Temporarily Revert "[OpenMP] Add Additional Function Attribute Information to… (authored by echristo).
Temporarily Revert "[OpenMP] Add Additional Function Attribute Information to…
Jul 17 2020, 3:07 PM
echristo added a reverting change for rG09fe0c5ab9ca: [OpenMP] Add Additional Function Attribute Information to OMPKinds.def: rG020545d386cf: Temporarily Revert "[OpenMP] Add Additional Function Attribute Information to….
Jul 17 2020, 3:07 PM
echristo committed rGae08dbc67326: Temporarily Revert "[InlineAdvisor] New inliner advisor to replay inlining from… (authored by echristo).
Temporarily Revert "[InlineAdvisor] New inliner advisor to replay inlining from…
Jul 17 2020, 2:59 PM
echristo added a reverting change for rG029946b11268: [InlineAdvisor] New inliner advisor to replay inlining from optimization remarks: rGae08dbc67326: Temporarily Revert "[InlineAdvisor] New inliner advisor to replay inlining from….
Jul 17 2020, 2:59 PM