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.

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

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

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.

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?

In D91050#2480906, @steven.zhang wrote:

Ping...

Thanks for the explanation, lgtm.

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.

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.

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

Also probably a good idea :)

I'd just seen that too. Thanks!

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.

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

In D89158#2363220, @aeubanks wrote:

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.

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. :)

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

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.

Seems reasonable to me.

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.

Awesome. Thanks for the update :)

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

Mark as requesting changes :)

Hi Peng,

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

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.

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

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

:)

In rG549e55b3d5634870aa9d42135f51ad46a6a0e347#948006, @thakis wrote:

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

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.

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. :)

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.

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

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

In D84108#2230308, @lebedev.ri wrote:

In D84108#2229882, @dmgreen wrote:

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?

In D84108#2228500, @echristo wrote:

In D84108#2227701, @lebedev.ri wrote:

In D84108#2227371, @echristo wrote:

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.

old.tar.xz27 KBDownload

new.tar.xz27 KBDownload

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?

In D84108#2229882, @dmgreen wrote:

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

In D84108#2229869, @dmgreen wrote:

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.

In D84108#2227709, @dmgreen wrote:

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

In D84108#2227701, @lebedev.ri wrote:

In D84108#2227371, @echristo wrote:

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?

One more follow up here:

In D86045#2227284, @MaskRay wrote:

In D86045#2227240, @echristo wrote:

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.

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.

I feel pretty OK approving this.

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

Hi All,

I'm also not sure this is passing?

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.

*thumbs up on ok for release*

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

In D84825#2195429, @MaskRay wrote:

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.

In D84825#2195362, @MaskRay wrote:

In D84825#2183197, @dblaikie wrote:

In D84825#2183130, @MaskRay wrote:

In D84825#2183080, @dblaikie wrote:

In D84825#2182964, @MaskRay wrote:

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In D84825#2182460, @dblaikie wrote:

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?

In D83273#2194842, @nickdesaulniers wrote:

In D83273#2194832, @craig.topper wrote:

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.

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

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

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.

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

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.