:)

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.

(Adding Sterling as well)
I think this will be an OK workaround for now, go ahead and reply on the main thread as I think they'll want to set the default runtime library as part of the toolchain. I'm surprised at this behavior as well.

Appears so, but ok.

In D83360#2154637, @aqjune wrote:

In D83360#2154584, @echristo wrote:

It's that even before the msan instrumentation the IR doesn't look correct - thus a miscompile.

I'll share a prototype of the InstSimplify patch on Phabricator, in a day or two; it would be great if the miscompilation is fixed with the patch.

It's that even before the msan instrumentation the IR doesn't look correct - thus a miscompile.

We're starting to see miscompiles as we do more testing as well, just nothing smaller at the moment.

This seems like something that we should then revert until we know that instsimplify can be updated with a fix?

LGTM.

Did you want to just change the pentium4 tuning? Otherwise naming things is hard and this feels awkward, but I don't have any better ideas :)

I may need to comment it better, but in this case part of it is that it's designed to only have the forced loop unroller run on it.

Some inline nits. I see you've already committed and that's fine - I still don't think we should do it, but we can delete it again soon :)

So the tuning here for SCE is also a "does not support" or something else?

In D83397#2139336, @schweitz wrote:

In D83397#2139202, @echristo wrote:

You also still haven't replied with "what bot/compiler/etc is going to break by making this change". Would you please do that?

Locally, we build flang with our changes with g++ and clang++ on a variety of Linux and MacOS.

In D83397#2139319, @schweitz wrote:

I've reverted these models from the upstreamed code for now. It will avoid the warnings.

In D83397#2139187, @schweitz wrote:

In D83397#2139131, @echristo wrote:

In D83397#2139111, @schweitz wrote:

Hi Eric,

There is an active development branch for the flang middle end. https://github.com/flang-compiler/f18-llvm-project/tree/fir-dev

That's not part of the llvm project.

That code base is being upstreamed piecemeal. Not all of the code is upstreamed at this point. It is simply a false impression that code in the middle of being upstreamed is "unused" or "unnecessary". Since it not all of it is upstreamed, changing interfaces and support code in llvm-project directly is going to cause problems that can become hard to track and resolve while the upstreaming is ongoing.

It very much is unused and unnecessary as there are no pieces of that code in the repository.

We are stuck between a rock and a hard place. Flang needs a middle end, as it currently can't generate code. Work on that middle end is being done. We are following the rules and trying to upstream that code in small "reviewable" chunks as quickly as possible. It's just simply going to be the case because of the interdependencies involved and the small chunks process that some code will merely appear to be temporally unused.

If you have a better solution to how to change the upstream process, then our group would be happy to hear it.

In D83397#2139111, @schweitz wrote:

Hi Eric,

There is an active development branch for the flang middle end. https://github.com/flang-compiler/f18-llvm-project/tree/fir-dev

In whose builds and why? What changes would you like? What compiler are you using? Can you provide any more information?

If this works then awesome. :)

Works for me :)