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

Constants #23

Merged
merged 2 commits into from Jan 30, 2019

Conversation

Projects
None yet
3 participants
@gabi-250
Copy link
Owner

gabi-250 commented Jan 29, 2019

The backend can now compile constants. This PR adds the implementation of some of the ConstMethods of the CodegenCx.

A SignedConst/UnsignedConst is a (typed) constant value. The SignedConst/UnsignedConsts are stored in the i_consts/u_consts vectors from the CodegenCx.

Value has two new variants which represent constants:

  • ConstUint(x) - x is the index of a constant in the u_consts vector
  • ConstInt(x) - x is the index of a constant in the i_consts vector

/// An unsigned constant.
///
/// It must be possible to represent this constant using 128 bits.

This comment has been minimized.

@ltratt

ltratt Jan 29, 2019

Collaborator

I think this comment means that value must fit within 128 bits (rather than the struct itself, which is clearly bigger than 128 bits)? Is there a particular reason for this? I wouldn't mind knowing what the consequences of ignoring this are (even if we just say "or the results are undefined").

This comment has been minimized.

@vext01

vext01 Jan 29, 2019

Collaborator

I guess there has to be an upper bound somewhere, as Rust doesn't do bignum out of the box.

This comment has been minimized.

@gabi-250

gabi-250 Jan 29, 2019

Author Owner

That's right, all integer constants must fit within 128 bits. In fact, this restriction is imposed by the ConstMethods trait the backend has to implement. All of the ConstMethods that produce integer constants take an argument which represents the value of the constant. This argument is always at most 128 bits wide.

@ltratt

This comment has been minimized.

Copy link
Collaborator

ltratt commented Jan 29, 2019

bors try

bors bot added a commit that referenced this pull request Jan 29, 2019

@vext01
Copy link
Collaborator

vext01 left a comment

A handful of minor comments.


/// An unsigned constant.
///
/// It must be possible to represent this constant using 128 bits.

This comment has been minimized.

@vext01

vext01 Jan 29, 2019

Collaborator

I guess there has to be an upper bound somewhere, as Rust doesn't do bignum out of the box.

use type_::Type;

/// An unsigned constant.
///

This comment has been minimized.

@vext01

vext01 Jan 29, 2019

Collaborator

I'd probably kill the blank comment lines to save vertical space.

ConstUndef,
BigConst(u128),
Global,
ConstUndef(Type),

This comment has been minimized.

@vext01

vext01 Jan 29, 2019

Collaborator

I'm intrigued. Where would an unspecified constant arise?

This comment has been minimized.

@gabi-250

gabi-250 Jan 29, 2019

Author Owner

As far as I am aware, this is very similar to the 'undef' values you can create in LLVM. Essentially, a ConstUndef is an uninitialized constant of an arbitrary type. The backend has to implement a const_undef function which returns an uninitialized Value.

There are several places where this is used. The backend-agnostic code from librustc_codegen_ssa calls const_undef to create unitialized structs. The structs can later be populated with values by calling insert_value(struct, element, index).

This comment has been minimized.

@vext01

vext01 Jan 29, 2019

Collaborator

Ah ok. "Uninitialised" makes complete sense. I was confused by "undefined", which is a scary word in the land of systems programming.

@@ -332,7 +365,17 @@ impl ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
}

fn const_to_opt_u128(&self, v: Value, sign_ext: bool) -> Option<u128> {
unimplemented!("const_to_opt_u128");
match v {
Value::ConstUint(i) => {

This comment has been minimized.

@vext01

vext01 Jan 29, 2019

Collaborator

For these one-line arms, you can kill the braces and move them onto one line.

match ... {
    Value::ConstUint(i) => Some(self.u_consts.borrow()[i].value),
    Value::ConstInt(i) => ...
@bors

This comment has been minimized.

Copy link

bors bot commented Jan 29, 2019

try

Build succeeded

@gabi-250

This comment has been minimized.

Copy link
Owner Author

gabi-250 commented Jan 29, 2019

Ready for re-review.

@vext01

This comment has been minimized.

Copy link
Collaborator

vext01 commented Jan 29, 2019

This looks fine to me, but since @ltratt chimed in, I'd like to check he is happy before continuing.

@ltratt

This comment has been minimized.

Copy link
Collaborator

ltratt commented Jan 29, 2019

Please squash.

@gabi-250 gabi-250 force-pushed the constants branch from 2650a63 to 7cc01c9 Jan 29, 2019

@gabi-250

This comment has been minimized.

Copy link
Owner Author

gabi-250 commented Jan 29, 2019

Done.

@ltratt

This comment has been minimized.

Copy link
Collaborator

ltratt commented Jan 29, 2019

bors r+

@bors

This comment has been minimized.

Copy link

bors bot commented Jan 29, 2019

👎 Rejected by code reviews

@vext01

vext01 approved these changes Jan 29, 2019

@vext01

This comment has been minimized.

Copy link
Collaborator

vext01 commented Jan 29, 2019

bors retry

bors bot added a commit that referenced this pull request Jan 29, 2019

Merge #23
23: Constants r=ltratt a=gabi-250

The backend can now compile constants. This PR adds the implementation of some of the `ConstMethods` of the `CodegenCx`.

A `SignedConst`/`UnsignedConst` is a (typed) constant value. The `SignedConst`/`UnsignedConst`s are stored in the `i_consts`/`u_consts` vectors from the `CodegenCx`.

`Value` has two new variants which represent constants:

* `ConstUint(x)` - x is the index of a constant in the `u_consts` vector
* `ConstInt(x)` - x is the index of a constant in the `i_consts` vector




Co-authored-by: Gabriela Alexandra Moldovan <gabi_250@live.com>
@bors

This comment has been minimized.

Copy link

bors bot commented Jan 29, 2019

@ltratt

This comment has been minimized.

Copy link
Collaborator

ltratt commented Jan 30, 2019

bors retry

@vext01

This comment has been minimized.

Copy link
Collaborator

vext01 commented Jan 30, 2019

Updating only changed submodules

And then it explodes. This is troublesome...

Let me read some more about buildbot and get back to you.

@vext01

This comment has been minimized.

Copy link
Collaborator

vext01 commented Jan 30, 2019

Ah, this may have actually been transient. Earlier in the logs:

Cloning into '/home/buildbot-worker3/bencher12-worker3/buildbot-build-script/build/src/doc/reference'...
error: RPC failed; HTTP 503 curl 22 The requested URL returned error: 503 Service Unavailable
fatal: The remote end hung up unexpectedly
fatal: clone of 'https://github.com/rust-lang-nursery/reference.git' into submodule path '/home/buildbot-worker3/bencher12-worker3/buildbot-build-script/build/src/doc/reference' failed
@vext01

This comment has been minimized.

Copy link
Collaborator

vext01 commented Jan 30, 2019

bors retry

@vext01

This comment has been minimized.

Copy link
Collaborator

vext01 commented Jan 30, 2019

bors ping

@bors

This comment has been minimized.

Copy link

bors bot commented Jan 30, 2019

pong

2 similar comments
@bors

This comment has been minimized.

Copy link

bors bot commented Jan 30, 2019

pong

@bors

This comment has been minimized.

Copy link

bors bot commented Jan 30, 2019

pong

@vext01

This comment has been minimized.

Copy link
Collaborator

vext01 commented Jan 30, 2019

The above is a bors bug I think. Notice how it got my prior retry after my ping, thus retrying the ping... I'll raise this on their support forum. But for now:

bors r+

bors bot added a commit that referenced this pull request Jan 30, 2019

Merge #23
23: Constants r=vext01 a=gabi-250

The backend can now compile constants. This PR adds the implementation of some of the `ConstMethods` of the `CodegenCx`.

A `SignedConst`/`UnsignedConst` is a (typed) constant value. The `SignedConst`/`UnsignedConst`s are stored in the `i_consts`/`u_consts` vectors from the `CodegenCx`.

`Value` has two new variants which represent constants:

* `ConstUint(x)` - x is the index of a constant in the `u_consts` vector
* `ConstInt(x)` - x is the index of a constant in the `i_consts` vector




Co-authored-by: Gabriela Alexandra Moldovan <gabi_250@live.com>
@bors

This comment has been minimized.

Copy link

bors bot commented Jan 30, 2019

@bors bors bot merged commit 7cc01c9 into master Jan 30, 2019

1 check passed

bors Build succeeded
Details

@bors bors bot deleted the constants branch Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment