Skip to content

[AArch64] Enable "sink-and-fold" in MachineSink by default #67432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

momchil-velikov
Copy link
Collaborator

No description provided.

@davemgreen
Copy link
Collaborator

You could remove the option from some of the tests now that it's on by default? Maybe not all of them, but it would be good if one of them tested the default.

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM

@momchil-velikov momchil-velikov merged commit ace20e2 into llvm:main Sep 27, 2023
@fpetrogalli
Copy link
Member

@momchil-velikov - I cannot find the comment I have attached to one of the commits in this PR. Just to make sure the information is not lost, I am letting you know that this PR seems to be breaking one of the public bots https://green.lab.llvm.org/green/job/clang-stage1-RA/35786/

momchil-velikov added a commit that referenced this pull request Sep 27, 2023
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
momchil-velikov added a commit that referenced this pull request Oct 6, 2023
…67432)"

This re-applies commit ace20e2, which was reverted in eff4ef2.

The issues were fixed in:

  * b30765c [AArch64] Fix an incorrect handling of debug values in
    MachineSink (#68107)

  * b454b04 [AArch64] Fix a compiler crash in MachineSink (#67705)
cmtice added a commit that referenced this pull request Oct 7, 2023
…default (#67432)""

This reverts commit a9d0ab2.
That commit is causing clang crashes.
@smeenai
Copy link
Collaborator

smeenai commented Oct 7, 2023

We're seeing some nice size wins in Meta's Android apps from this change (0.3% reduction in overall native library size). Looking forward to the reland :)

momchil-velikov added a commit that referenced this pull request Oct 13, 2023
…67432)'

This re-applies commit a9d0ab2, which
was reverted by 8abb2ac.

The issue was fixed by 7510f32
@momchil-velikov momchil-velikov deleted the sink-and-fold branch October 13, 2023 12:53
aemerson added a commit that referenced this pull request Oct 15, 2023
…default (#67432)'"

This reverts commit dbb9fae.

This seems to cause miscompiles on CTMark/sqlite3 and others with GISel.
momchil-velikov added a commit that referenced this pull request Oct 19, 2023
…67432)'

This reverts revert 1950507.

An issue was fixed in bea3684
and some newly appeared tests updated.
@bgra8
Copy link
Contributor

bgra8 commented Oct 24, 2023

@momchil-velikov this is causing a substantial compilation time regression when generating dwarf output. A V8 c++ file which compiles in less than 10s before this revision, compiles in 10 minutes at this revision.

The compilation command is something like:

clang
  -cc1 -triple aarch64-generic-linux-gnu -emit-obj \
  -debug-info-kind=constructor \
  -split-dwarf-file /tmp/repro.dwo \
  -split-dwarf-output /tmp/repro.dwo \
  -O3 -std=gnu++20 \
  -fsized-deallocation \
  -o /tmp/repro.o \
  -x c++ repro.cc

I started a reduction but it might take considerable time as we deal with such a high compilation time regression.

A revert will be welcome to unblock us before we come up with the reduced test case.

@bgra8
Copy link
Contributor

bgra8 commented Oct 28, 2023

@momchil-velikov as I reduce the file even further I get to less and less of a visible slow-down between this and previous compiler, so I had to stop the process as it was taking a lot of time.

With the attached reduced repro the slowdown is now 5x. I hope you find the issue. Please let me know if you need more info.

repro.cc.zip

@momchil-velikov
Copy link
Collaborator Author

@bgra8 Thanks, I was able to observe 10x difference between -g0 and -g3.

@momchil-velikov
Copy link
Collaborator Author

Small reproducer here:

int f(int);

void g(int x) {
  int y = x + 1;

  int t0 = y;
  f(t0);

  int t1 = y;
  f(t1);
}

With these commands

#! /bin/bash

./bin/clang -target aarch64-linux -g -O2 -S ccc.c -emit-llvm
./bin/llc  ccc.ll --aarch64-enable-sink-fold=true --stop-before=machine-sink -o ccc-before.mir
./bin/llc  ccc.ll --aarch64-enable-sink-fold=true --stop-after=machine-sink -o ccc-after.mir
kdiff3 ccc-{before,after}.mir

it's quite evident how sink-and-fold causes quadratic explosion in the number the DBG_VALUE instructions.

@momchil-velikov
Copy link
Collaborator Author

Easier to see here: https://gcc.godbolt.org/z/Tv878a39T
One issue is the debug info is replicated, a second issues it's sometimes lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants