Bug 228261 - Request for a specific llvm commit to be merged into current and llvm60 port
Summary: Request for a specific llvm commit to be merged into current and llvm60 port
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: amd64 Any
: --- Affects Some People
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-15 03:09 UTC by Shane
Modified: 2018-05-20 16:04 UTC (History)
3 users (show)

See Also:
dim: mfc-stable11+


Attachments
sample clang6 pre-processor output (77.16 KB, application/x-xz)
2018-05-16 18:45 UTC, Shane
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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.