chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.79k stars 421 forks source link

nil-returning function is not executed #14640

Open vasslitvinov opened 4 years ago

vasslitvinov commented 4 years ago

Codegen can discard calls to functions that return dtNil. For example:

proc f() {
  writeln("in f()");   // not executed
  return nil;
}

proc g(arg) {
  writeln("in g()");
}

g(f());

The culprit for the above is this code in compiler/codegen/expr.cpp

// in codegenAssign()
    if (from.chplType == dtNil)
    {
      if (to_ptr.chplType->symbol->hasEitherFlag(FLAG_WIDE_REF, FLAG_WIDE_CLASS))
      {
        from = codegenWideHere(codegenNullPointer(), to_ptr.chplType);
        type = to_ptr.chplType->getValType();
        from.isLVPtr = GEN_VAL;
      } else {
        from = codegenNullPointer();
        type = to_ptr.chplType->getValType();
        from.isLVPtr = GEN_VAL;
      }
    }

This file has also these potential trouble-makers:

DEFINE_PRIM(PRIM_EQUAL) {
    if (call->get(1)->typeInfo()->symbol->hasFlag(FLAG_WIDE_CLASS) &&
        call->get(2)->typeInfo()->symbol->hasFlag(FLAG_WIDE_CLASS)) {
    ....
    } else if (call->get(1)->typeInfo()->symbol->hasFlag(FLAG_WIDE_CLASS) &&
               call->get(2)->typeInfo() == dtNil) {
      ret = codegenIsZero(call->get(1));

    } else if (call->get(2)->typeInfo()->symbol->hasFlag(FLAG_WIDE_CLASS) &&
               call->get(1)->typeInfo() == dtNil) {
      ret = codegenIsZero(call->get(2));

    } else {
    ....

and analogously for PRIM_NOTEQUAL. These are easy to fix ex. by checking also that call->get(.) is a SymExpr.

bradcray commented 4 years ago

Huh... what breaks if that conditional is removed? What do the git logs say about why it was introduced?

vasslitvinov commented 4 years ago

The else branch was added in #13584 "Fix NUMA after #13447", with this message:

The code generator was emitting a memcpy with &NULL. This commit adjusts the code generator to generate nil with GEN_VAL to avoid that situation in this case. The code generator already handled this case in the event that the destination was a wide pointer.

The if itself was added in r23375, with this explanation:

Support was added for the assignment of nil to a wide class pointer (FLAG_WIDE). To obtain correct code (and keep the backend C compiler happy), nil must be cast to a wide class object (FLAG_WIDE_CLASS) of the correct type. That is it must be the value type of the wide class reference type of the LHS argument.

What does this tell us about how to fix it?

I know that in the --local case from.c is &nil when it is a nil literal, and f() in my example. So there we can distinguish the two cases. Although I don't know how to codegen if it is f(). In the --no-local case it is something like chpl_macro_17. Not sure for the NUMA case.