Skip to content

Commit

Permalink
Merge pull request chapel-lang#7659 from mppf/real32-coerce
Browse files Browse the repository at this point in the history
Improve resolution of coercions & enums

This PR:
 * Enables param coercions from int/real/complex to real and complex when the value is perfectly representable
 * Enables param handling of real/imag/complex
    + (but in order for that to be user-facing, we'd need param operator overloads in ChapelBase - the subject of PR chapel-lang#7668).
 * Significantly improves enum behavior, so that enums can coerce to integral types in which the enum constants will fit.

What's the headline behavior improvement?
 * the literal 0.0 can coerce to real(32)
 * the literal 1 can coerce to real(32)

Resolves chapel-lang#5922.

### Summary of Behavior Changes

Here are some top-level features that continue to work as they did before:
* param i = 1:uint(32) + 1;  results in y.type being uint(32)
* integer literals such as 1 prefer to resolve to 'int', if other arguments did not indicate a stronger preference
* int, uint, enum, bool param/literals resolve to "default size" when crossing type category (e.g. bool to int(?w)) but otherwise prefer whichever overload is smallest (e.g. int(8) will prefer int(16) over int(64)).

Here are some top-level features that work after this PR (and not before):
* 0.0, 1, or other numeric values that are exactly representable can pass to a real(32) argument
     + for param reals, that means verifying both the mantissa and exponent will fit
* int(8), int(16) non-param values can pass to a real(32) argument (since they are exactly representable)
* param r = 1:real(32) + 0.0;  results in r.type being real(32) (after we add real param ops in PR chapel-lang#7688)
* enum X { a, b, c = a + b } works at all
* enum Y { a=1 }    var  x:myenum;   f(x); can now dispatch to f(any integral type), since the enum value fits into any integral type. This works even without resulting in ambiguity if overloads for all integer types are present!
* real param/literals resolve to "default size" when crossing type but otherwise prefer whichever overload is smallest
* 15 futures retired!

### Enum Examples that Behave Differently

``` chapel
      enum myenum { v }

      proc foo(x:uint(8)) {
        writeln("uint(8)");
      }

      proc foo(x:uint) {
        writeln("uint");
      }

      var x:myenum;

      foo(x);
```

After this PR, this resolves to the 'uint' foo; on master it fails to resolve, with ambiguity. I think it makes sense to prefer the "default sized type" in this case (even before this PR, we would prefer an 'int' version), but more importantly adding this rule helps to prevent surprising ambiguity errors that arise from complex relationships between the various most-specific-argument rules.

``` chapel
      enum myenum { X=1, Y=1000 }

      proc foo(x:int(8)) {
        writeln("int(8) version");
      }

      proc foo(x:int(32)) {
        writeln("int(32) version");
      }

      foo(myenum.X);
```

After this PR, this resolves to "int(32) version" rather than "int(8) version". In fact it could resolve to the int(8) version if it were the only option, but the int(32) version is preferred because the enum type, myenum, which is the type of myenum.X, can always dispatch to int(32), but myenum.X can only dispatch to the int(8) version because it happens to be a param with the value 1.

### Testing and Review

Passes full local futures testing.
Reviewed by @vasslitvinov - thanks!

### Future Work

 * consider also enabling coercions from int(32) to real(32) (discuss in issue chapel-lang#7740)
 * actually adding 'real' param functions (PR chapel-lang#7688 is a start)
 * spec changes to describe actual behavior of "param prefers" in the resolution section (note though that some of the features, such as passing an int(8) variable to a real(32) argument, are already described as something that should work in the spec. Other areas, such as how function disambiguation works with param arguments, are incorrectly described in the spec.).
  • Loading branch information
mppf authored Nov 15, 2017
2 parents 9fcf392 + 21ce703 commit e4d6a19
Show file tree
Hide file tree
Showing 95 changed files with 2,577 additions and 990 deletions.
21 changes: 0 additions & 21 deletions STATUS.devel
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,6 @@ General (see also portability section at the bottom of this file)

Types, Params, Variables, and Consts
------------------------------------
- Enum types always represented using the smallest integer type
available, even if the enum values require a larger integer.
# functions/bradc/resolution/badEnumDispatch.future
- Enum member initializations cannot depend on previous enum members
# types/enum/nspark/dependentElements.future
- Compile time integer operations on enum values may result in non-param values
# users/kreider/bug_enum.future
- The default value for the locale type is incorrect.
# types/locale/bradc/defaultLocaleVal.future
# multilocale/sungeun/locale_default.future
Expand Down Expand Up @@ -153,20 +146,6 @@ Types, Params, Variables, and Consts

Conversions
-----------
- Implicit conversions of enums to uint not supported.
# types/enum/vass/enum-to-uint-1.future
- Casts and relational operations involving enum constants may fail to
compile or produce incorrect answers.
# types/enum/sungeun/cast_enum_weird_2.future
# types/enum/sungeun/cast_expr_init_1.future
# types/enum/sungeun/cast_expr_init_2.future
# types/enum/sungeun/cast_neg_start_1.future
# types/enum/sungeun/cast_neg_start_2.future
# types/enum/sungeun/cast_neg_start_3.future
# types/enum/sungeun/cast_neg_start_oor_1.future
# types/enum/sungeun/cast_neg_start_oor_2.future
# functions/diten/param_enum_to_int.future
# functions/diten/param_enum_to_string.future
- Casts to non-type variables do not result in an error at compile time.
# expressions/vass/cast-to-non-type-1.future
# expressions/vass/cast-to-non-type-2.future
Expand Down
46 changes: 24 additions & 22 deletions compiler/AST/expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1794,50 +1794,52 @@ void NamedExpr::accept(AstVisitor* visitor) {

bool
get_int(Expr *e, int64_t *i) {
Immediate* imm = NULL;
if (e) {
if (SymExpr *l = toSymExpr(e)) {
if (VarSymbol *v = toVarSymbol(l->symbol())) {
if (v->immediate) {
if (v->immediate->const_kind == NUM_KIND_INT) {
*i = v->immediate->int_value();
return true;
}
}
}
imm = getSymbolImmediate(l->symbol());
}
}

if (imm && imm->const_kind == NUM_KIND_INT) {
*i = imm->int_value();
return true;
}

return false;
}

bool
get_uint(Expr *e, uint64_t *i) {
Immediate* imm = NULL;
if (e) {
if (SymExpr *l = toSymExpr(e)) {
if (VarSymbol *v = toVarSymbol(l->symbol())) {
if (v->immediate) {
if (v->immediate->const_kind == NUM_KIND_UINT) {
*i = v->immediate->uint_value();
return true;
}
}
}
imm = getSymbolImmediate(l->symbol());
}
}

if (imm && imm->const_kind == NUM_KIND_UINT) {
*i = imm->uint_value();
return true;
}

return false;
}

bool
get_string(Expr *e, const char **s) {
Immediate* imm = NULL;
if (e) {
if (SymExpr *l = toSymExpr(e)) {
if (VarSymbol *v = toVarSymbol(l->symbol())) {
if (v->immediate && v->immediate->const_kind == CONST_KIND_STRING) {
*s = v->immediate->v_string;
return true;
}
}
imm = getSymbolImmediate(l->symbol());
}
}

if (imm && imm->const_kind == CONST_KIND_STRING) {
*s = imm->v_string;
return true;
}

return false;
}

Expand Down
18 changes: 18 additions & 0 deletions compiler/AST/symbol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2759,6 +2759,24 @@ VarSymbol *new_ImmediateSymbol(Immediate *imm) {
return s;
}

Immediate *getSymbolImmediate(Symbol* sym) {
Immediate* imm = NULL;

if (VarSymbol* var = toVarSymbol(sym)) {
imm = var->immediate;
}
if (EnumSymbol* enumsym = toEnumSymbol(sym)) {
EnumType* et = toEnumType(enumsym->type);
// If the integer type is not yet known, the enum type
// hasn't been resolved.
INT_ASSERT(et->getIntegerType() != NULL);
imm = enumsym->getImmediate();
}

return imm;
}


// enable locally-unique temp names?
bool localTempNames = false;

Expand Down
Loading

0 comments on commit e4d6a19

Please sign in to comment.