GPUOpen-Drivers / llvm-dialects

LLVM Dialects Library
Apache License 2.0
22 stars 22 forks source link

Op's create methods support passing an instruction name #80

Closed JanRehders-AMD closed 8 months ago

JanRehders-AMD commented 8 months ago

Equivalent to CallInst, for more readable code. This allows to pass in a value name without a call to setName which is especially useful when directly returning a create op.

For example:

return m_builder->create<ExampleOp>(m_builder->getInt32(123), "example_123");

JanRehders-AMD commented 8 months ago

@nhaehnle @Flakebi could you add yourself as reviewers? Didn't do that when creating the PR and don't have enough rights to add you later

tsymalla commented 8 months ago

You need to update the generated files for the Example dialect for this to pass CI.

nhaehnle commented 8 months ago

As @tsymalla-AMD pointed out, you do need to fix the CI failures here.

tsymalla commented 8 months ago

As @tsymalla-AMD pointed out, you do need to fix the CI failures here.

You just need to take the ExampleDialect.h.inc and ExampleDialect.cpp.inc, which should be generated as part of the check-llvm-dialects target, and put it in the generated/ folder for the CI to pass.

tsymalla commented 8 months ago

Please add some tests (as mentioned above) that shows the name of the instruction is correctly set, and that this plays correctly with various cases. Thanks.

JanRehders-AMD commented 8 months ago

@tsymalla There was indeed a naming conflict causing this to not compile. I've added a fix for that (separate commit so it's easier to review but will be squashed) plus a test.

Here is the code produced by the conflict test I've added:

class InstNameConflictTestOp : public ::llvm::CallInst {
  static const ::llvm::StringLiteral s_name; //{"test.try.conflict"};

public:
  static bool classof(const ::llvm::CallInst *i) {
    return ::llvm_dialects::detail::isSimpleOperation(i, s_name);
  }
  static bool classof(const ::llvm::Value *v) {
    return ::llvm::isa<::llvm::CallInst>(v) &&
           classof(::llvm::cast<::llvm::CallInst>(v));
  }
  static InstNameConflictTestOp *create(::llvm_dialects::Builder &b,
                                        ::llvm::Value *instName,
                                        ::llvm::Value *inst_Name,
                                        const llvm::Twine &inst__name = "");

  bool verifier(::llvm::raw_ostream &errs);

  ::llvm::Value *getInstName();
  void setInstName(::llvm::Value *instName);
  ::llvm::Value *getInst_Name();
  void setInst_Name(::llvm::Value *inst__name);
};

and from the test single-vararg.td:

class SingleVarArgOp : public ::llvm::CallInst {
  static const ::llvm::StringLiteral s_name; //{"test.return"};

public:
  static bool classof(const ::llvm::CallInst *i) { return ::llvm_dialects::detail::isSimpleOperation(i, s_name); }
  static bool classof(const ::llvm::Value *v) {
    return ::llvm::isa<::llvm::CallInst>(v) && classof(::llvm::cast<::llvm::CallInst>(v));
  }
  static SingleVarArgOp *create(::llvm_dialects::Builder &b, ::llvm::ArrayRef<::llvm::Value *> args,
                                const llvm::Twine &inst__name = "");

  bool verifier(::llvm::raw_ostream &errs);

  ::llvm::iterator_range<::llvm::User::value_op_iterator> getArgs();
};
tsymalla-AMD commented 8 months ago

LGTM (you need to resolve the merge conflict)

JanRehders-AMD commented 8 months ago

Please add some tests (as mentioned above) that shows the name of the instruction is correctly set, and that this plays correctly with various cases. Thanks.

From MainExample.cpp

  b.create<xd::HandleGetOp>("name.of.llvm.value");
  b.create<xd::InstNameConflictOp>(b.getInt32(1));
  b.create<xd::InstNameConflictOp>(b.getInt32(1), "name.foo");
  b.create<xd::InstNameConflictDoubleOp>(b.getInt32(1), b.getInt32(2));
  b.create<xd::InstNameConflictDoubleOp>(b.getInt32(1), b.getInt32(2), "bar");
  SmallVector<Value *> moreVarArgs = varArgs;
  b.create<xd::InstNameConflictVarargsOp>(moreVarArgs);
  b.create<xd::InstNameConflictVarargsOp>(moreVarArgs, "two.varargs");
  moreVarArgs.push_back(b.getInt32(3));
  b.create<xd::InstNameConflictVarargsOp>(moreVarArgs, "three.varargs");
  moreVarArgs.push_back(b.getInt32(4));
  b.create<xd::InstNameConflictVarargsOp>(moreVarArgs, "four.varargs");

produces

  %name.of.llvm.value = call target("xd.handle") @xd.handle.get()
  %19 = call i32 (...) @xd.inst.name.conflict(i32 1)
  %name.foo = call i32 (...) @xd.inst.name.conflict(i32 1)
  %20 = call i32 (...) @xd.inst.name.conflict.double(i32 1, i32 2)
  %bar = call i32 (...) @xd.inst.name.conflict.double(i32 1, i32 2)
  %21 = call i32 (...) @xd.inst.name.conflict.varargs(ptr %p1, i8 %p2)
  %two.varargs = call i32 (...) @xd.inst.name.conflict.varargs(ptr %p1, i8 %p2)
  %three.varargs = call i32 (...) @xd.inst.name.conflict.varargs(ptr %p1, i8 %p2, i32 3)
  %four.varargs = call i32 (...) @xd.inst.name.conflict.varargs(ptr %p1, i8 %p2, i32 3, i32 4)
  ret void