Skip to content
This repository has been archived by the owner on Jun 20, 2019. It is now read-only.

Set more call attributes and fnspec on all library functions. #617

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Feb 11, 2018

Rationale for call attributes.

  • ECF_COLD: On functions that should never be called (_d_assert, _d_switch_error, etc...)
  • ECF_LEAF: On functions that don't call postblit() or destroy() (_d_allocmemory, etc...)
  • ECF_NOTHROW: On functions that never throw (__gdc_begin_catch())
  • ECF_MALLOC: On functions have a malloc-like call (_d_allocmemory).
  • ECF_PURE: On functions that return a "new" pointer to GC only. (_d_allocmemory, others should work, but are a bit risky)
  • ECF_CONST: On functions that depend solely on its arguments (_d_switch_string, etc...)

Explanation of the fnspec string:

  • First character is the return spec.
    • m: Return value has no aliases
    • 1: First argument same as return value
    • 3: Third argument same as return value
    • .: Have no idea what is returned.
  • Rest of the characters in the string represent each argument passed.
    • R: Argument is readonly, not dereferenced recursively, and does not escape.
    • r: Argument is readonly, and does not escape.
    • W: Argument is written to, not dereferenced recursively, and does not escape.
    • w: Argument is written to, but does not escape.
    • .: Have no idea what may be done with argument.

@ibuclaw ibuclaw force-pushed the flagspec branch 4 times, most recently from 14d376d to 9c67c18 Compare February 12, 2018 01:20
@ibuclaw ibuclaw force-pushed the flagspec branch 2 times, most recently from 93ae3e1 to d0982a7 Compare February 14, 2018 19:34
@ibuclaw
Copy link
Member Author

ibuclaw commented Feb 14, 2018

Removed the "returns" flag for runtime functions that return an array. Ran into a backend bug that generated wrong codegen. The rest should be fine.

@ibuclaw
Copy link
Member Author

ibuclaw commented Feb 15, 2018

Made the following amendments:

  1. _d_arraycatnTX: second argument R -> r, because it's a byte[][], and requires access to both levels.

  2. _d_arrayctor: second argument R -> ., may need to do this for others where there's a from and to parameter. Just because the from should be read-only, it is only shallow copied, and the postblit() call after memcpy() may change some interior pointer inside from.

  3. _d_newitemT: return flag m -> ., because new'ing a data type that has a dtor() doesn't really have no aliases. The GC reference means that the dtor() will be called on gc_term(), but if the result is unused, it won't be constructed properly.
    Have also applied the same to _d_newitemiT, but may need to reconsider this flag, or check more deeply on a case-by-case basis whether having an alias - but pretending there are none - is really worthwhile.

  4. Commented out test10094 in sdtor and marked it as XBUG, as it's a valid failure - as all NRVO pointer comparison tests are really undefined behaviour in the current scheme - we should manage NRVO ourselves by creating a hidden parameter for the return value if we really want to mimic DMD.
    The NRVO failure has been exposed by marking _adEq2 as .RRR - albeit indirectly, as its really the assert before calling _adEq2() that triggers. My guess is that as the compiler now sees that adEq2 doesn't change the value of any globals or locals in the function, the first assert must always be false (the test is pretty much the same as test17457 in the same test file).

@ibuclaw
Copy link
Member Author

ibuclaw commented Feb 15, 2018

  1. Have applied the same as point (3) to _d_newclass and _d_arrayliteralTX, for the same reason that post-construction was being removed as dead code.

@jpf91
Copy link
Contributor

jpf91 commented Feb 15, 2018

What's the rationale to implement this? Do you expect large performance / optimization benefits using the attributes? And it seems NRVO is a never-ending story...

@ibuclaw
Copy link
Member Author

ibuclaw commented Feb 15, 2018

What's the rationale to implement this? Do you expect large performance / optimization benefits using the attributes? And it seems NRVO is a never-ending story...

  • Rationale:
    Initially I wanted a way to eliminate dead GC allocations in the codegen. Seems like that isn't possible though in gcc at the moment.

Currently there is a special case in the middle end that recognises certain malloc-like builtins and removes them if the LHS is eliminated by the DCE. Whereas what would be more desirable is for that to be a call return flag to handle generically.

  • Performance:
    Yes, there's performance and optimization benefits, as partially proven by the now failing test. Because the implementation detail of runtime functions are unknown to the compiler, giving it hints about what the function does means it won't be treated as a black hole in the optimizer.

I.e: _adEq2 does not modify or escape any of its arguments, does not modify global memory. Therefore if we encounter (explanatory example):

a = b;
_adEq2(typeid(a), &a, &c);
assert(a == b);

The optimizer can infer that the assert can never be false.

This sort of implication however may not be without its downsides, so may need a little more scrutiny over the initial values that I set.

  • NRVO:
    Probably best to raise this (again) with upstream. What we need is clarity over whether or not the pointer comparison tests for nrvo are undefined behaviour. If they are precisely defined, then this must be part of the language spec, possibly with an inclusion of what makes a type a candidate for nrvo.

@ibuclaw
Copy link
Member Author

ibuclaw commented Feb 15, 2018

  • NRVO:
static S* p;
S fn()
{
  S tmp;
  p = &tmp;
  return tmp;
}
S s = fn();
assert(p == &s);

Let me be clear that the value of p is undefined because the address of tmp is now out of scope.

@UplinkCoder
Copy link

LGTM.

@ibuclaw ibuclaw closed this Dec 26, 2018
@ibuclaw ibuclaw deleted the flagspec branch December 26, 2018 15:51
@ibuclaw ibuclaw restored the flagspec branch December 26, 2018 15:51
@ibuclaw ibuclaw reopened this Dec 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants