Skip to content
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

aggregate, reduce the complexity of the loop checking the overlapped fields #9288

Merged
merged 1 commit into from Jan 24, 2019
Merged

aggregate, reduce the complexity of the loop checking the overlapped fields #9288

merged 1 commit into from Jan 24, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 23, 2019

Based on the fact that overlapping is only possible in unions and anonymous unions (yes ?) it's possible to avoid a nested control loop iff

  1. the aggregate is not an union.
  2. the aggregate doesn't contain any field declared in an anonymous union.

Little drawback : the second condition requires to add a field to the VarDeclaration class since AnonDeclaration (i.e anonymous union or anonymous struct) is implemented as an attribute.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @Basile-z! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9288"

@dlang-bot dlang-bot merged commit f17b9a6 into dlang:master Jan 24, 2019
@ghost ghost deleted the aggr-overlap-check branch January 24, 2019 10:09
dlang-bot added a commit that referenced this pull request Jan 24, 2019
fixup #9288, missing c++ counterpart to VarDecl class
merged-on-behalf-of: Petar Kirov <[email protected]>
@kinke
Copy link
Contributor

kinke commented Feb 18, 2019

AFAICT this led to a regression, some overlapping fields in the following example apparently don't get their VarDeclaration.overlapped flag set:

struct SWithUnion
{
    char c;
    S nested;

    union
    {
        struct { ubyte ub = 6; ushort us = 33; align(8) ulong ul_dummy = void; ulong last = 123; }
        struct { uint ui1; uint ui2 = 84; ulong ul = 666; }
    }
}

@kinke
Copy link
Contributor

kinke commented Feb 18, 2019

  1. the aggregate doesn't contain any field declared in an anonymous union

This goes more than one level deep, that's the problem, this code just looks at the direct variable children of anonymous unions (and thus skips the 2 nested anonymous structs in the example). It would have to descend into nested anonymous unions and structs too.

I take it this was an attempt to improve performance (with any noticeable gains?); having to descend into subtrees etc. just makes this more complex and brittle, so I recommend a revert for 2.085.

kinke added a commit to kinke/ldc that referenced this pull request Feb 18, 2019
@WalterBright
Copy link
Member

@kinke can you turn this into a bugzilla issue, and add a piece of code that fails under this PR?

@kinke
Copy link
Contributor

kinke commented Feb 18, 2019

I'm swamped with LDC work.

@WalterBright
Copy link
Member

I understand, but you do have a test case that triggers the problem, it just needs to be purified.

@WalterBright
Copy link
Member

@kinke I created a bugzilla for this, https://issues.dlang.org/show_bug.cgi?id=19685 can you see about the test case?

@kinke
Copy link
Contributor

kinke commented Feb 18, 2019

Done, thx for bugzillaing.

@ghost
Copy link
Author

ghost commented Feb 19, 2019

Well, revert seems to be appropriated here since the commit was supposed to do the same thing.

@ghost
Copy link
Author

ghost commented Feb 19, 2019

The problem is that align was not handled at all by this PR, so again, revert.

@wilzbach
Copy link
Member

Link to the revert PR: #9373

kinke added a commit to kinke/ldc that referenced this pull request Feb 27, 2019
kinke added a commit to kinke/ldc that referenced this pull request Mar 1, 2019
clrpackages pushed a commit to clearlinux-pkgs/ldc that referenced this pull request Apr 8, 2019
Ivan Butygin (2):
      jit: Move BindPtr to public (#3030)
      Jit: better error handling and handle correctly __chkstk function (#3051)

Martin Kinkelin (103):
      Update CHANGELOG.md
      Initialize Azure Pipelines CI
      Set up CI with Azure Pipelines
      Azure CI: Add Win32 job
      Azure CI: Add package artifact
      Azure CI: Add Windows multilib job
      Azure CI: Use LLVM with enabled assertions for untagged builds
      Azure CI: Adjust artifact filenames for tagged builds
      Azure CI: Upload artifacts to GitHub release
      Azure CI: Don't litter src directory, add comments, ...
      Azure CI: Add Linux multilib & macOS x64 jobs
      Azure CI: Perform shallow git clone
      Azure CI: Misc. fixups
      TEMP: Use LLVM prebuilt by Azure CI
      Azure CI: Hack around druntime test failure for macOS 10.13
      Remove AppVeyor CI
      Rededicate CircleCI
      Azure CI: Hack around full LTO integration test failure on macOS
      README.md: Replace AppVeyor badge by Azure one
      Azure CI: Set num parallel build/test jobs for Linux/Mac to num available CPU cores
      Update README.md
      Azure CI: Fix lit installation on Windows (#3008)
      Upgrade front-end & libs to v2.085.0-beta.1
      Work around `enum TOK` C++ mangling issue/regression
      Fix union regression: Revert dlang/dmd#9288
      Refactoring: Let mars.d handle cmdline reconciliation
      Fix tests/codegen/ptr_16_bit.d
      Add cmdline option -checkaction=context
      Add cmdline option -verrors-context (-verrors=context for LDMD)
      Add cmdline option -extern-std=c++{98,11,14,17}
      Try to restore compilability with ltsmaster
      druntime: Fix fwd declaration signature for precise GC
      Add cmdline options -preview and -revert
      Print transition/preview/revert usage help without src file
      Change behavior when invoking LDC/LDMD without source files
      dmd-testsuite: Disable a few new tests for LDC, incl. 'unit tests'
      Hide legacy cmdline options -dip<N>
      LDMD: Support -mcpu=h and -mcpu=help too
      Sync LDMD usage help with DMD's
      Upgrade to v2.085.0-rc.1
      druntime: Allow overriding register_default_gcs() at link-time
      druntime: Fix linking issues with stdcpp stand-alone tests
      Disable druntime test 'gc' for shared-only druntime
      dmd-testsuite: Disable most Objective-C tests
      Disable dcompute lit-tests for now
      Advance phobos/dmd-testsuite properly (#3011)
      TEMP: Use early LDC-LLVM v8.0 incl. SPIRV-LLVM-Translator
      Adapt to latest LLVM 8 changes
      Restore -disable-fp-elim option for LLVM 8+
      CMake: Find LLVMSPIRVLib library if built in-tree
      Hide a few new LLVM 8 cmdline options
      Travis: Switch macOS SPIR-V job to vanilla LLVM
      CMake: Simplify & fix LLD header detection
      ShippableCI: Sync/streamline with Azure, use prebuilt host compiler
      TEMP: Use early LDC-LLVM v8.0 for ShippableCI too
      dmd-testsuite: Tweak for better parallelization
      Parallelize compilation of LDC D unittests
      Update LLVM 8 llvm-profdata and FileCheck
      Enable opt-in GC for front-end via explicit -lowmem switch
      Keep AST expressions of DMD-style inline asm arguments alive
      Upgrade bundled dub to v1.14.0 (#3012)
      Keep IrFuncTyArg::type alive
      dmd-testsuite: Add rudimentary -lowmem tests
      Filter out --DRT-* options in the cmdline
      Re-enable cross-module-inlining for `pragma(inline, true)`
      Phobos: Force-inline more functions with __FILE__ as template parameter
      Don't exclude functions in object.d from cross-module inlining
      README.md: Switch Docker cmdline to Seb's Ubuntu images
      Upgrade to LLVM v8.0.0-rc4
      Fix crash with -lowmem -mixin
      Shippable: Disable LTO integration tests (failing assertions with LLVM 8)
      README.md: Use nicer badges & add badge for latest release (#3016)
      Travis: Upgrade to Ubuntu 16.04
      Azure & Shippable: Use LLVM without lib{tinfo,edit} dependencies
      CMake: Get rid of manual libtinfo/libpthread dependencies
      Azure CI: Try to make tags trigger automatic builds
      Azure CI: Work around GitHubRelease task overwriting isPreRelease flag
      Travis: Skip dmd-testsuite (release) on Mac to reduce time-outs
      Azure CI fixup
      dmd-testsuite: Parallelize building of tools (#3022)
      Use semver-friendly version for untagged builds
      SemaphoreCI: Use -lowmem when building dmd-testsuite tools in parallel
      Travis: Don't build with coverage instrumentation
      Travis: Skip dmd-testsuite-debug, only run release one
      CI: Switch to LDC-LLVM v8.0.0 final
      Azure: Yet another attempt at making tags trigger builds
      dmd-testsuite: Adapt & extend runnable/test13774.sh
      MSVC: Add {oldnames,legacy_stdio_definitions}.lib to default C libs (#3036)
      Don't emit object.RTInfo(Impl) instantiations in dcompute modules
      Revert "Disable dcompute lit-tests for now"
      Azure CI: Make prebuilt Linux package usable on Ubuntu 14.04 again (#3038)
      Travis: Add Linux LLVM 8.0 job & switch one Mac job to LLVM 8.0
      Azure CI: Upgrade clang for Windows jobs to v8.0
      Merge upstream stable (58878aeb) (#3039)
      Refactoring: Remove obsolete AttrBuilder wrapper
      Support generic ldc.attributes.llvmAttr UDAs for function parameters
      Only enforce SSSE3 (with -O) for LLVM 7.x (#3045)
      Auto-detect Visual Studio 2019+ (#3044)
      druntime: Add @restrict parameter UDA
      Propagate well-known length of newly allocated arrays (#3042)
      Upgrade to v2.085.1
      Add declaration context to semantic errors
      Changelog: Add v1.15.0

Temtaime (1):
      Add lib files when linking for a static library
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.

7 participants