[MLIR][LLVMIR][DLTI] Add #llvm.target, #llvm.data_layout and TargetAttrInterface

[rolfmorel]

No description provided.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[rolfmorel]

@rengolin, what's the ownership model of TargetMachines? Should this attribute become the owner of this TargetMachine * by using a unique_ptr?

[rengolin]

TargetMachine can be put into a unique_ptr, then reset using an llvm::Target::createTargetMachine and then use .get() to pass it to the LLVM pipeline builder.

[rolfmorel]

I remembered why I didn't do it before: wrapping TargetMachine in a unique_ptr makes ODS-generated code, in particular a printer, scream:

In file included from /data/nfs_home/rmorel/llvm-project.git/llvm-target-dlti/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp:20:
/data/nfs_home/rmorel/llvm-project.git/llvm-target-dlti/llvm/include/llvm/ADT/TypeSwitch.h:102:29: error: call to implicitly-deleted copy constructor of 'mlir::LLVM::TargetAttr'
      result.emplace(caseFn(caseValue));
                            ^~~~~~~~~
tools/mlir/include/mlir/Dialect/LLVMIR/LLVMOpsAttrDefs.cpp.inc:372:80: note: in instantiation of function template specialization 'llvm::TypeSwitch<mlir::Attribute, llvm::LogicalResult>::Case<mlir::LLVM::TargetAttr, (lambda at tools/mlir/include/mlir/Dialect/LLVMIR/LLVMOpsAttrDefs.cpp.inc:372:111)>' requested here
  return ::llvm::TypeSwitch<::mlir::Attribute, ::llvm::LogicalResult>(def)    .Case<::mlir::LLVM::TargetAttr>([&](auto t) {
                                                                               ^
tools/mlir/include/mlir/Dialect/LLVMIR/LLVMOpsAttrDefs.h.inc:306:55: note: copy constructor of 'TargetAttr' is implicitly deleted because field 'targetMachine' has a deleted copy constructor
  std::optional<std::unique_ptr<llvm::TargetMachine>> targetMachine = std::nullopt;
                                                      ^
/opt/rh/gcc-toolset-13/root/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/optional:707:7: note: copy constructor of 'optional<std::unique_ptr<llvm::TargetMachine>>' is implicitly deleted because base class '_Enable_copy_move<is_copy_constructible_v<unique_ptr<TargetMachine, default_delete<TargetMachine>>>, __and_v<is_copy_constructible<unique_ptr<TargetMachine, default_delete<TargetMachine>>>, is_copy_assignable<unique_ptr<TargetMachine, default_delete<TargetMachine>>>>, is_move_constructible_v<unique_ptr<TargetMachine, default_delete<TargetMachine>>>, __and_v<is_move_constructible<unique_ptr<TargetMachine, default_delete<TargetMachine>>>, is_move_assignable<unique_ptr<TargetMachine, default_delete<TargetMachine>>>>, optional<unique_ptr<TargetMachine, default_delete<TargetMachine>>>>' has a deleted copy constructor
      private _Enable_copy_move<
      ^
/opt/rh/gcc-toolset-13/root/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/enable_special_members.h:160:15: note: '_Enable_copy_move' has been explicitly marked deleted here
    constexpr _Enable_copy_move(_Enable_copy_move const&) noexcept  = delete;
              ^
tools/mlir/include/mlir/Dialect/LLVMIR/LLVMOpsAttrDefs.cpp.inc:372:120: note: passing argument to parameter 't' here
  return ::llvm::TypeSwitch<::mlir::Attribute, ::llvm::LogicalResult>(def)    .Case<::mlir::LLVM::TargetAttr>([&](auto t) {
                                                                                                                       ^
1 error generated.

This is with std::optional<std::unique_ptr<llvm::TargetMachine>> targetMachine = std::nullopt; in the extraClassDeclaration and

  auto targetMach = std::unique_ptr<llvm::TargetMachine>(target->createTargetMachine(
          llvm::Triple(getTriple().strref()), getChip().strref(),
          getFeatures().getFeaturesString().c_str(), {}, {}));

  targetMachine = { std::move(targetMach) };

in the implementation.

If I am understanding correctly, the issue seems to be that the type switch in the following code is trying to do a copy of the attribute, even though attributes should just be thought of/treated as (unique) pointers (... if my understanding is correct):

static ::llvm::LogicalResult generatedAttributePrinter(::mlir::Attribute def, ::mlir::AsmPrinter &printer) {
  return ::llvm::TypeSwitch<::mlir::Attribute, ::llvm::LogicalResult>(def)    .Case<::mlir::LLVM::TargetAttr>([&](auto t) {
      printer << ::mlir::LLVM::TargetAttr::getMnemonic();
t.print(printer);
      return ::mlir::success();
    })
    .Case<::mlir::LLVM::DataLayoutAttr>([&](auto t) {
      printer << ::mlir::LLVM::DataLayoutAttr::getMnemonic();
t.print(printer);
      return ::mlir::success();
    })
    .Case<::mlir::LLVM::CConvAttr>([&](auto t) 
    ...

[rolfmorel]

Hopefully someone could help explain what the right way is to hang owned pointers off of an attribute.

Any and all pointers welcome!

[rolfmorel]

@rengolin, I am not sure what we want to do for verification here. Do we want to insist on the specified triple, cpu, and target_features being valid together. And do we verify that through constructing the relevant TargetMachine?

How would that work for dealing with IR which is annotated with a #llvm.target specifying a target that your current llvm-project isn't built with. Is it right that verification fails in that case? (Maybe it is, but it feels wrong as now verification succeeding or failing becomes a function of whether you built MLIR with the right flags.)

[rengolin]

One problem we faced on our side earlier was that it's not that easy to know you're constructing an invalid TargetMachine (link). But validating all possible combinations is just not viable. For downstream targets, you'd have to overload some verification to match their alternatives.

Other tools don't verify much either (other than completely crash if the strings are invalid), so I guess we can start with that. The discussion about which combinations are valid (link) are generally higher in the pipeline (front-end). At the IR level, we can only treat them as "gospel" and crash if we really can't build a TargetMachine.