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.

Sign up for GitHub

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

Jump to bottom

Continue with DValue refactoring and fix issue #1327 #1562

Merged
merged 2 commits into from
Jun 26, 2016
Merged

Continue with DValue refactoring and fix issue #1327 #1562

merged 2 commits into from
Jun 26, 2016

Conversation

kinke
Copy link
Member

@kinke kinke commented Jun 15, 2016

  • Introduce a DRValue base class to be able to discriminate between DLValues and DRValues (e.g., function parameters).
  • Let DValue::getRVal() return each DValue's value as DRValue. This allows to convert a DLValue to a DRValue while retaining the D type, something we've previously lost when returning the low-level rvalue directly.
  • Let the DtoR/LVal() helpers be the only way to convert a DValue to a low-level value.

@kinke
Copy link
Member Author

kinke commented Jun 15, 2016

The 2nd commit demonstrates what I was aiming for with that DValue refactoring, but it's only a beginning, there are surely more issues of this kind. #1327 is fixed with this at least.

@kinke
Copy link
Member Author

kinke commented Jun 15, 2016

The 3rd commit fixes #1433.

@kinke kinke changed the title Continue with DValue refactoring Continue with DValue refactoring and fix a few evaluation order issues Jun 15, 2016
@JohanEngelen
Copy link
Member

Could you add some tests for these fixes?

@kinke
Copy link
Member Author

kinke commented Jun 16, 2016 •
edited
Loading

Yeah I'll add some tests. They should probably go upstream.

Edit: ldc-developers/dmd-testsuite#23

@kinke kinke mentioned this pull request Jun 17, 2016
Closed
@kinke
Copy link
Member Author

kinke commented Jun 18, 2016

I've moved the #1433 fix to its own PR #1566, as it is yet unclear whether that's a bug or not.

@kinke kinke changed the title Continue with DValue refactoring and fix a few evaluation order issues Continue with DValue refactoring and fix issue #1327 Jun 18, 2016
@kinke
Copy link
Member Author

kinke commented Jun 20, 2016 •
edited
Loading

I'm not perfectly happy with DLValue::getRVal() creating a new DImValue instance on the heap, but I surely don't want to change everything to a std::shared_ptr<DValue> or something like this.
I do really like the ability to convert a DLValue to a DRValue representing its current state though, as it allows us to work even more with DValues (instead of LLValues) in the future. Keeping the D type around and the info whether it's an l- or rvalue is nice and eliminates unclear assumptions about a stand-alone LLValue.

@kinke
Continue with DValue refactoring
7fec36b 
* Introduce a DRValue base class to be able to discriminate between
  DLValues and DRValues (e.g., function parameters).
* Let DValue::getRVal() return each DValue's value as DRValue.
  This allows to convert a DLValue to a DRValue, a snapshot of the
  lvalue's current state, while retaining the D type, something we've
  previously lost when returning the low-level rvalue directly.
* Let the DtoR/LVal() helpers be the only way to convert a DValue to a
  low-level value.
@kinke
Copy link
Member Author

kinke commented Jun 25, 2016

Any concerns or has noone found the time to review this yet?

@JohanEngelen
Copy link
Member

I haven't reviewed this in detail, but I have no concerns :)

dnadlinger
dnadlinger reviewed Jun 26, 2016
View reviewed changes
gen/dvalue.h

// base class for d-values
/// Associates an immutable low-level value with an immutable D type.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: "immutable" is a bit misleading imho (in any other context, "immutable D type" would refer to the immutable keyword – maybe An immutable pair of LLVM value and associated D type.?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very well, done.

kinke
kinke reviewed Jun 26, 2016
View reviewed changes
gen/dvalue.h
class DImValue : public DValue {
/// Represents an immediate D value via a low-level rvalue.
/// Suited for primitive low-level types like pointers (incl. class references),
/// integral and floating-point types.
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to elaborate a bit here, but my understanding of 'immediate' is pretty vague too. ;)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks – I think pretty much all there even is to understand is that it is a D rvalue represented by a straight LLVM value, and without any special further properties (e.g. being null, etc).

@dnadlinger
Copy link
Member

lgtm (modulo tests still running, of course)

@kinke
Fix evaluation/load order of binary operators
a3adb40 
Fixes issue ldc-developers#1327 by loading immediately from lvalues resulting from
left- and right-hand sides.
kinke
kinke reviewed Jun 26, 2016
View reviewed changes
gen/dvalue.h
/// Represents an immediate D value (simple rvalue with no special properties
/// like being a compile-time constant) via a low-level rvalue.
/// Restricted to primitive types such as pointers (incl. class references),
/// integral and floating-point types.
Copy link
Member Author

Choose a reason for hiding this comment

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

I've incorporated your latest comment here.

@kinke
Copy link
Member Author

kinke commented Jun 26, 2016

The tests were green and I've only touched some comments. Merging eagerly. dmd-testsuite test will follow soon.

@kinke kinke merged commit a2fdffa into ldc-developers:master Jun 26, 2016
@kinke kinke deleted the dvalue branch June 26, 2016 19:35
@kinke
Copy link
Member Author

kinke commented Jun 26, 2016

Test: ldc-developers/dmd-testsuite@030de81

@JohanEngelen
Copy link
Member

git bisect tells me that this PR (specifically commit a3adb40) is causing a unittest failure in Weka's codebase.
I'll look into whether this is because of a bug in Weka's code, or a new codegen bug. The code is involving threads/fibers, if that is a good hint for anyone.

@JohanEngelen
Copy link
Member

I found the bugged code. It looks like a bug in Weka's code to me. Need to discuss with the author.

@JohanEngelen JohanEngelen mentioned this pull request Jul 11, 2016
Closed
@JohanEngelen
Copy link
Member

Found the problem: #1617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kinke @JohanEngelen @dnadlinger