Problem with FuseOps (and embedded constants in TIR)

kparzysz · February 24, 2022, 11:47pm

Long story short, after https://github.com/apache/tvm/pull/8509 we see a lot of floating point models crash during compilation for Hexagon. On Hexagon (downstream) we use the mixed-precision pass to convert float32 data to float16. Float16 constants are not supported by constants in TIR and compilation aborts.

Longer story…

The relay pass FuseOps is the one that can extract constants out of expressions, and replace them with parameters. Constants that are not parameters cannot have type float16, because that type is not supported by current TIR code (for embedded constants). If this extraction doesn’t happen, compilation will abort if a float16 constant is found in TIR.

After commit b5f1dabce4 (PR8509), FuseOps will no longer extract constants if the build target has link_params set to true. This can be, at least in theory, overridden by pass context flag relay.FuseOps.link_params, but there is an issue.

The problem is as follows:

The FoldConstant pass runs, and it creates a fresh PassContext, without any config flags in it. It also uses the “cpu” target instead of the actual one for CompilationConfig.
During execution, FoldConstant will invoke relay interpreter’s function Eval, which initiates a series of relay passes, including FuseOps.
FuseOps gets link_params value from IRModule's attribute Executor, which is still consistent with the actual target. If the original target has link_params = 1, it will take effect regardless of any flags added to PassContext at the relay.Build time.

It seems like FuseOps should be getting settings consistent with the CPU target that FoldConstants created, instead of using Executor from IRModule. This difference may have further consequences if passes executed during FoldConstants consult the executor for more information.

Any thoughts?

mbs-octoml · February 25, 2022, 12:33am

Agree with your last sentence – FoldConstants should be CPU only and not carry forward any target-specific flags. (Ideally do all that more directly instead of piggy-backing on the interpreter, but that’s a bigger issue.)

masahi · February 25, 2022, 1:11am

I wonder why TIR constants doesn’t support fp16? Because of the need for c-codegen? @manupa-arm

Hzfengsy · February 25, 2022, 2:06am

I’m not sure. But I guess it is because C++ doesn’t have a native fp16 type support?

manupa-arm · February 25, 2022, 10:03am

Hi @kparzysz ,

Sorry to hear that there was downstream failure because of #8509.

I am also wondering how this is true, because of the following (note that we just used what was supported in TVM via LinkedParam node) :

github.com

apache/tvm/blob/dcbdeddba290caee4f604e678a7e315aea038881/src/target/llvm/codegen_params.cc#L137-L157


case runtime::DataType::TypeCode::kFloat:
  switch (arr_type.bits()) {
    case 16:
      // NOTE: float16 is treated as uint16_t.
      element_type = llvm::Type::getIntNTy(*ctx, arr_type.bits());
      BuildLLVMVector<uint16_t>(element_type, arr->data, num_elements, &elements);
      break;
    case 32:
      element_type = llvm::Type::getFloatTy(*ctx);
      BuildLLVMVector<float>(element_type, arr->data, num_elements, &elements);
      break;
    case 64:
      element_type = llvm::Type::getDoubleTy(*ctx);
      BuildLLVMVector<double>(element_type, arr->data, num_elements, &elements);
      break;
    default:
      CHECK(false) << "CodegenParams: only support 32- or 64-bit floating point; saw "
                   << arr_type.bits() << "-bit array";
      break;
  }
  break;

github.com

apache/tvm/blob/dcbdeddba290caee4f604e678a7e315aea038881/src/target/source/codegen_params.cc#L214-L229


case runtime::DataType::TypeCode::kFloat: {
  os.fill(' ');
  os.setf(std::ios::left, std::ios::adjustfield);
  if (arr_type.bits() == 16) {
    // NOTE: print types not widely supported by C as uint16_t.
    PrintIntegralArray<uint16_t>(arr->data, num_elements, indent_chars, os);
  } else if (arr_type.bits() == 32) {
    PrintFloatingPointArray<float>(arr->data, num_elements, indent_chars, os);
  } else if (arr_type.bits() == 64) {
    PrintFloatingPointArray<double>(arr->data, num_elements, indent_chars, os);
  } else {
    CHECK(false) << "CodegenParams: only support 32- or 64-bit floating point; saw "
                 << arr_type.bits() << "-bit array";
  }
  break;
}

The above are called respectively in the following locations :

github.com

apache/tvm/blob/dcbdeddba290caee4f604e678a7e315aea038881/src/target/llvm/codegen_llvm.cc#L1485


    builder_->CreateCondBr(cond, then_block, end_block, md_very_likely_branch_);
    builder_->SetInsertPoint(then_block);
    this->VisitStmt(op->then_case);
    builder_->CreateBr(end_block);
  }
  builder_->SetInsertPoint(end_block);
}


void CodeGenLLVM::VisitStmt_(const AllocateConstNode* op) {
  auto data = op->data.value();
  auto array = NDArrayToLLVMArray(ctx_, data);
  std::string symbol_name = op->buffer_var->name_hint;
  llvm::GlobalVariable* param_symbol = new llvm::GlobalVariable(
      *module_, array->getType(), true, llvm::GlobalValue::InternalLinkage, array, symbol_name);


  var_map_[op->buffer_var.operator->()] = param_symbol;
  this->VisitStmt(op->body);
}


void CodeGenLLVM::VisitStmt_(const AllocateNode* op) {
  ICHECK(!is_zero(op->condition));

github.com

apache/tvm/blob/dcbdeddba290caee4f604e678a7e315aea038881/src/target/source/codegen_c.cc#L673


              << "extern \"C\" {\n"
              << "#endif\n"
              << "static const ";


  PrintType(data.DataType(), decl_stream);


  // Allocate the global static variable
  decl_stream << " __attribute__((section(\".rodata.tvm\"), "
              << "aligned(" << constants_byte_alignment_->value << "))) " << symbol_name << "["
              << num_elements << "] = {\n";
  NDArrayDataToC(data, 4, decl_stream);


  decl_stream << "};\n"
              << "#ifdef __cplusplus\n"
              << "}  // extern \"C\"\n"
              << "#endif\n";
  var_idmap_[op->buffer_var.operator->()] = symbol_name;
  this->PrintStmt(op->body);
}


void CodeGenC::VisitExpr_(const LoadNode* op, std::ostream& os) {  // NOLINT(*)

We need to fix this if it is not happenning. @kparzysz would you be able to file an issue ? attn : @dmitriy-arm @Mousius

I think we need to move the link-params to be a property of the TIR backend (i.e. target).

However, the above seems to be the workaround of the issue, the real issue I suspect is hexagon backend expect LinkedParams node if --link-params is used, however, there is not a unit test (a simple codegen one) to assert this – that should have been broken by #8509.

kparzysz · February 25, 2022, 2:57pm

We were hitting this assertion:

github.com

apache/tvm/blob/main/src/relay/backend/te_compiler_cache.cc#L242


          return make_const(dtype, static_cast<const int32_t*>(data)[0]);
        } else if (dtype == DataType::Int(64)) {
          return make_const(dtype, static_cast<const int64_t*>(data)[0]);
        } else if (dtype == DataType::Float(32)) {
          return make_const(dtype, static_cast<const float*>(data)[0]);
        } else if (dtype == DataType::Float(64)) {
          return make_const(dtype, static_cast<const double*>(data)[0]);
        } else if (dtype == DataType::Bool()) {
          return make_const(dtype, static_cast<const uint8_t*>(data)[0]);
        } else {
          LOG(FATAL) << dtype << " not handled";
          return tvm::PrimExpr();
        }
      },
      "compile_engine_const", topi::kBroadcast);
  scalars_.push_back(value->op);
  return {value};
} else {
  const auto* ttype = op->checked_type().as<TensorTypeNode>();
  std::stringstream ss;
  ss << "constant_" << const_index++;

We do have a unit testcase that checks the LLVM IR to see if the _linked_param function was generated, but I’m not sure if it runs in CI right now, since it requires Hexagon target to be enabled. This is actually work in progress, so these tests should start running soon.

manupa-arm · February 25, 2022, 3:22pm

@kparzysz .

As mentioned in the PR, the above reference is about scalar constants, that is not subject to link-params. (Correct me if I am wrong – @dmitriy-arm ).

#8509 is about non-scalar constants.

One option is to hexagon backend needs to be adjusted to handle AllocateConst nodes, instead of LinkedParams node.

Lets continue the discussion on the PR (#8509) to avoid duplicate conversation. Do you agree @kparzysz ?

kparzysz · February 25, 2022, 3:24pm

Sure, I replied in the PR.