[Mlir-commits] [mlir] [MLIR][Python] Remove pybind workaround for not throwing IndexError (PR #174139)

[llvmlistbot at llvm.org]

MaPePeR wrote:

I'm not really a benchmarking person, but I gave it a shot.

<details><summary>Benchmark attempt script</summary>

```python
import timeit

from mlir.ir import Context, Location, Module, InsertionPoint, Block, Region, OpView
from mlir.dialects import func, builtin, scf, arith

def generate_module():
    m = Module.create()
    with InsertionPoint(m.body):
        f = func.FuncOp("main", builtin.FunctionType.get([], []))
    with InsertionPoint(f.body.blocks.append()):
        generate_ops(10, 2)
        func.ReturnOp([])
    return m

def generate_ops(count: int, depth: int):
    if depth == 0:
        return
    lower = arith.ConstantOp(builtin.IntegerType.get_signless(64), 0)
    upper = arith.ConstantOp(builtin.IntegerType.get_signless(64), 100)
    step =  arith.ConstantOp(builtin.IntegerType.get_signless(64), 1)
    for i in range(count):
        forop = scf.ForOp(lower, upper, step)
        with InsertionPoint(forop.region.blocks[0]):
            generate_ops(count, depth - 1)
            scf.YieldOp([])

def walk_module(m: Module):
    walk_block(m.body)

def walk_region(region: Region):
    for block in region.blocks:
        walk_block(block)

def walk_block(block: Block):
    for predecessors in block.predecessors:
        pass
    for successors in block.successors:
        pass
    for op in block.operations:
        walk_op(op)

def walk_op(op: OpView):
    for result in op.results:
        pass
    for successors in op.successors:
        pass
    for operands in op.operands:
        pass
    for region in op.regions:
        walk_region(region)

with Context(), Location.unknown():
    m = generate_module()

    #  From timeit.main:
    t = timeit.Timer(lambda: walk_module(m))
    number, _ = t.autorange()
    repeats = 5
    raw_timings = t.repeat(repeats, number)
    timings = [dt / number for dt in raw_timings]
    best = min(timings)
    print(f"{number} loops, best of {repeats}: {best * 1000:.3g} msecs per loop")
```
</details>

Without this change (b77f7df4971dcbfd9b4490574845ac45c9489e76):
```
50 loops, best of 5: 5.97 msecs per loop
```

With this change (a92eeaab6242f20f0980829b339fcdfbc8534c28): 
```
50 loops, best of 5: 6.35 msecs per loop
```

With this change, but `throw nanobind::index_error(...)` instead of `PyErr_SetString`:
```
50 loops, best of 5: 8.27 msecs per loop
```

I don't really have a good benchmarking environment, so This is in a docker container in WSL with a Debug build with shared libs...

But from the result its seems like this change makes the code ~6% slower, so its probable not going to be merged. Still a lot better than the 38% runtime increase from throwing exceptions, but significantly slower than the current version nevertheless.

So I'm going to close this.

>From this result we should really look into replacing all the other exceptions, though.

https://github.com/llvm/llvm-project/pull/174139