[Transform][BUG] fix gc-check fail on creating arith pass

[crazydemo]

As Title.

[Menooker]

What is the type of sourceTypeStrs?

[crazydemo]

::mlir::Pass::ListOption<std::string>

[Menooker]

No, I searched that:

struct ArithEmulateUnsupportedFloatsOptions {
  ::llvm::ArrayRef<std::string> sourceTypeStrs;
  std::string targetTypeStr = "f32";
};

Your code is still dangerous. Please note that llvm::ArrayRef<std::string> does not hold the ownership of temp value = SmallVector<std::string>{"bf16"};, so after options.sourceTypeStrs = SmallVector<std::string>{"bf16"};, it still point to a dead value.

Suggested:

std::string data = "bf16";
sourceTypeStrs = &data;

[ZhennanQin]

I don't see much difference. std::string data lives on stack and will be constructed after function returns. But we need it alive during pm.run(op).

[crazydemo]

maybe we can use:

static ArrayRef<std::string> data = {"bf16"};
options.sourceTypeStrs = data;

[Menooker]

actually we don't need to save a SmallVector<std::string> data. A single string is enough? Maybe ```

const static string data = "...";
options.sourceTypeStrs = {&data, 1};

[crazydemo]

This is enough for now, changed.

[Menooker]

Sorry to revisit the issue here. I finally found that we only need

std::array<std::string, 1> typestr = {"bf16"};
options.sourceTypeStrs = typestr ;

Note that we don't need static storage here.

@crazydemo is right in some way. I am sorry for misunderstanding the MLIR code. We pass the arrayref to the options struct ArithEmulateUnsupportedFloatsOptions. And the struct is passed to createArithEmulateUnsupportedFloats. In TD-generated source build/tools/mlir/include/mlir/Dialect/Arith/Transforms/Passes.h.inc, we can see it calls Pass constructor

  ArithEmulateUnsupportedFloatsBase(const ArithEmulateUnsupportedFloatsOptions &options) : ArithEmulateUnsupportedFloatsBase() {
    sourceTypeStrs = options.sourceTypeStrs;
    targetTypeStr = options.targetTypeStr;
  }

while sourceTypeStrs is a field of ArithEmulateUnsupportedFloatsBase of type ::mlir::Pass::ListOption<std::string> sourceTypeStrs{ ....}

ListOption extends an ancestor class which has std::vector to store the data. So ArithEmulateUnsupportedFloatsBase::sourceTypeStrs copies the data from ArithEmulateUnsupportedFloatsOptions::sourceTypeStrs, and it holds the ownership of the data. See the ancestor class:

//llvm/include/llvm/Support/CommandLine.h
template <class DataType> class list_storage<DataType, bool> {
  std::vector<DataType> Storage;
  std::vector<OptionValue<DataType>> Default;
  bool DefaultAssigned = false;
 ...
};

In conclusion, we don't need a static lifetime bf16 string here.

[Menooker]

BTW, please always avoid writing code like

ArrayRef<std::string> x = {"abc"};
somefunc(x);

Please note that "abc" has type of char[4] which is not std::string. The compiler inserts an conversion here, and the code looks like

ArrayRef<std::string> x = {std::string("abc")};

Also note std::string("abc") is a temp value and the lifetime ends at the full evaluation of the statement, which means it is dead after the ; at this line. And x outlives the temp value and points to a dead pointer.

However, the following code is valid:

ArrayRef<const char*> x = {"abc"};
ArrayRef<StringRef> y = {"abc"};

Below also valid

void somefunc(ArrayRef<std::string> x);
....
somefunc({"bf16"});

[ZhennanQin]

Good finding! The interface design doesn't have a problem.