Bug 228261

Summary:

Request for a specific llvm commit to be merged into current and llvm60 port

Product:

Base System

Reporter:

Shane <FreeBSD>

Component:

bin

Assignee:

freebsd-bugs (Nobody) <bugs>

Status:

Closed FIXED

Severity:

Affects Some People

CC:

brooks, dim, emaste

Priority:

---

Flags:

dim: mfc-stable11+

Version:

CURRENT

Hardware:

amd64

OS:

Any

Attachments:

Description	Flags
sample clang6 pre-processor output	none

Description Shane 2018-05-15 03:09:46 UTC

I am the port maintainer for devel/godot

The automated port builds for current have been emailing me for a while about a build failure of the godot port. Turns out that one particular file takes over 1.5 hours to compile and is normally killed as a runaway. My poudriere builds don't kill runaways and I hadn't noticed the 2 hour build times.

The issue is related to an optimisation, so should be a more general issue than one ports code base. Using -O0 bypasses this issue but -O1 or higher hits it. It is also amd64 specific, adding -m32 bypasses the issue.

This build issue has been noticed by other godot developers (on osx and debian), and a commit has made to llvm-master that resolves it which has not been merged into the 6.0 release. Using clang++-devel from llvm-devel-7.0.d20180225 does not hit this issue.

The same patch should be applied to current as well as the llvm60 port.

The llvm commit that reportedly fixes this is
https://github.com/llvm-project/llvm-project-20170507/commit/595304ad70df4db24a803b5ea3edc2acd8f850d6

The godot project issue related to this is
https://github.com/godotengine/godot/issues/18023

You don't need to build the entire godot port to test this, you can start building the port, then stop it after the first files compile and just compile the one file, if it takes more than 10 seconds you have hit the issue.

cd /usr/ports/devel/godot
make
Ctrl-C
cd work/godot-3.0.2-stable
clang++60 -o test.o -O3 -I. -Icore -Icore/math -Iplatform/x11 servers/physics/collision_solver_sat.cpp

Comment 1 commit-hook freebsd_committer

2018-05-15 17:50:53 UTC

A commit references this bug:

Author: brooks
Date: Tue May 15 17:50:20 UTC 2018
New revision: 470040
URL: https://svnweb.freebsd.org/changeset/ports/470040

Log:
  Merge r322325 from upstream.  This allows devel/godot to build in a
  reasionable abount of time:

  PeepholeOpt cleanup/refactor; NFC

  - Less unnecessary use of `auto`
  - Add early `using RegSubRegPair(AndIdx) =` to avoid countless
    `TargetInstrInfo::` qualifications.
  - Use references instead of pointers where possible.
  - Remove unused parameters.
  - Rewrite the CopyRewriter class hierarchy:
     - Pull out uncoalescable copy rewriting functionality into
       PeepholeOptimizer class.
     - Use an abstract base class to make it clear that rewriters are
       independent.
  - Remove unnecessary \brief in doxygen comments.
  - Remove unused constructor and method from ValueTracker.
  - Replace UseAdvancedTracking of ValueTracker with DisableAdvCopyOpt use.

  PR:		228261
  Reported by:	FreeBSD@ShaneWare.Biz

Changes:
  head/devel/llvm60/Makefile
  head/devel/llvm60/files/patch-svn-r322325

Comment 2 Brooks Davis freebsd_committer

2018-05-15 17:53:41 UTC

BTW, the llvm-project repo on github is some random person AFACT and has a git export that doesn't match git.llvm.org.  If you find yourself needing to send links to llvm git commits in the future, the llvm-mirror project is a better choice.

Comment 3 Dimitry Andric freebsd_committer

2018-05-15 17:59:30 UTC

I'm still having a look at this particular change, and I contacted the author, because his commit message explicitly mentions "NFC" (which is LLVM parlance for No Functional Change).  So it's rather strange that this fixes a hang.

I'll wait a while until I get some form of response, otherwise I'll import the fix and put it on the MFC list.

Comment 4 Shane 2018-05-16 14:13:56 UTC

Well that commit may not mean to make a change, but I can compile devel/llvm60 with and without that one patch and it changes the compile time of the specified file.

The change may be subtle, but it makes a difference, but then the suggested change to the godot code reuses an old variable instead of creating a new one and it also makes a difference. Maybe the change triggers a different optimisation result which changes the final binary.

Comment 5 Shane 2018-05-16 18:45:57 UTC

Created attachment 193465 [details]
sample clang6 pre-processor output

Now that I know what the issue is, I realise that it can be tested by using the preprocessor output without the need of the entire godot project.

By sending this to clang++ v6.0 with a "-O1" or higher the extended compile time can be seen. There is a linking error at the end but that isn't relevant to this.

Comment 6 commit-hook freebsd_committer

2018-05-17 14:39:02 UTC

A commit references this bug:

Author: dim
Date: Thu May 17 14:38:58 UTC 2018
New revision: 333715
URL: https://svnweb.freebsd.org/changeset/base/333715

Log:
  Pull in r322325 from upstream llvm trunk (by Matthias Braun):

    PeepholeOpt cleanup/refactor; NFC

    - Less unnecessary use of `auto`
    - Add early `using RegSubRegPair(AndIdx) =` to avoid countless
      `TargetInstrInfo::` qualifications.
    - Use references instead of pointers where possible.
    - Remove unused parameters.
    - Rewrite the CopyRewriter class hierarchy:
       - Pull out uncoalescable copy rewriting functionality into
         PeepholeOptimizer class.
       - Use an abstract base class to make it clear that rewriters are
         independent.
    - Remove unnecessary \brief in doxygen comments.
    - Remove unused constructor and method from ValueTracker.
    - Replace UseAdvancedTracking of ValueTracker with DisableAdvCopyOpt
      use.

  Even though upstream marked this as "No Functional Change", it does
  contain some functional changes, and these fix a compiler hang for one
  particular source file in the devel/godot port.

  PR:		228261
  MFC after:	3 days

Changes:
  head/contrib/llvm/lib/CodeGen/PeepholeOptimizer.cpp

Comment 7 commit-hook freebsd_committer

2018-05-20 16:03:34 UTC

A commit references this bug:

Author: dim
Date: Sun May 20 16:03:21 UTC 2018
New revision: 333926
URL: https://svnweb.freebsd.org/changeset/base/333926

Log:
  MFC r333715:

  Pull in r322325 from upstream llvm trunk (by Matthias Braun):

    PeepholeOpt cleanup/refactor; NFC

    - Less unnecessary use of `auto`
    - Add early `using RegSubRegPair(AndIdx) =` to avoid countless
      `TargetInstrInfo::` qualifications.
    - Use references instead of pointers where possible.
    - Remove unused parameters.
    - Rewrite the CopyRewriter class hierarchy:
       - Pull out uncoalescable copy rewriting functionality into
         PeepholeOptimizer class.
       - Use an abstract base class to make it clear that rewriters are
         independent.
    - Remove unnecessary \brief in doxygen comments.
    - Remove unused constructor and method from ValueTracker.
    - Replace UseAdvancedTracking of ValueTracker with DisableAdvCopyOpt
      use.

  Even though upstream marked this as "No Functional Change", it does
  contain some functional changes, and these fix a compiler hang for one
  particular source file in the devel/godot port.

  Approved by:	re (kib)
  PR:		228261

Changes:
_U  stable/11/
  stable/11/contrib/llvm/lib/CodeGen/PeepholeOptimizer.cpp

Comment 8 Dimitry Andric freebsd_committer

2018-05-20 16:04:52 UTC

Fixed in head and stable/11. This will be part of 11.2-RELEASE.