search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.84k stars 11.47k forks source link

Missing `self.` in `opVariadicEqualPrefixTemplate` in mlir-tblgen #101132

Closed niejiutao closed 5 days ago

niejiutao commented 1 month ago

In the template string defined at

https://github.com/llvm/llvm-project/blob/3fcc4f28ed808d72cb3c6bc45f9bca891ae5ca48/mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp#L148

there should be self. before operation.operands. Otherwise, the mlir-tblgen will generate python code like this:

  @builtins.property
  def inputs(self):
    start, pg = _ods_equally_sized_accessor(operation.operands, 2, 0, 0)
    return self.operation.operands[start:start + pg]

causing "NameError: name 'operation' is not defined".

llvmbot commented 1 month ago

@llvm/issue-subscribers-mlir

Author: None (niejiutao)

In the template string defined at https://github.com/llvm/llvm-project/blob/3fcc4f28ed808d72cb3c6bc45f9bca891ae5ca48/mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp#L148 there should be `self.` before `operation.operands`. Otherwise, the mlir-tblgen will generate python code like this: ``` @builtins.property def inputs(self): start, pg = _ods_equally_sized_accessor(operation.operands, 2, 0, 0) return self.operation.operands[start:start + pg] ``` causing "NameError: name 'operation' is not defined".
kasper0406 commented 2 weeks ago

@niejiutao , I hit this issue as well. Wondering if you are working on a PR to get it resolved?

kasper0406 commented 2 weeks ago

I got a bit deeper in this. I think the missing self. issue described here, is just the first thing going wrong. Next it will actually go into the _ods_equally_sized_accessor function, where it tries to calculate:

total_variadic_length = len(elements) - n_variadic + 1

I believe this calculation is wrong, and I think it should instead be:

total_variadic_length = len(elements) - n_preceding_simple

My assumption is that simple elements will only take up 1 space, and that we only care about the preceding simple elements.

niejiutao commented 1 week ago

@niejiutao , I hit this issue as well. Wondering if you are working on a PR to get it resolved?

No, we just workaround it by computing operand index manually. Thank you for your fix.

makslevental commented 1 week ago

We can close this right?

kasper0406 commented 5 days ago

At least from my perspective, this should be fine to close.