String caching causes incorrect results with mutable instructions

[trbabb]

Certain instructions (like Phi) can be mutated. If they are stringified at any point (including for debugging), subsequent mutations won't be reflected in the SSA, giving incorrect results and very confusing bugs!

It's probably strictly better for those instructions to invalidate their string caches immediately upon mutation.

I have a PR for this: https://github.com/numba/llvmlite/pull/661

[esc]

@trbabb thank you for raising this issue.

[siboehm]

I just ran into this, here's a self-contained example (modified from examples/):

from llvmlite import ir
from llvmlite.ir.transforms import Visitor

# Create some useful types
double = ir.DoubleType()
fnty = ir.FunctionType(double, (double, double))

# Create an empty module...
module = ir.Module(name=__file__)
# and declare a function named "fpadd" inside it
func = ir.Function(module, fnty, name="fpadd")

# Now implement the function
block = func.append_basic_block(name="entry")
builder = ir.IRBuilder(block)
a, b = func.args
c = ir.Constant(ir.IntType(bits=1), False)
result = builder.select(c, a, b, name="res", flags=["nsz"])
builder.ret(result)


class VisitSelect(Visitor):
    def visit_Instruction(self, instr):
        if instr.opname != "select":
            return

        print("Before:", instr)
        instr.flags = ["fast"]
        print("After: ", instr)


# Print the module IR
before = str(module)
VisitSelect().visit(module)
after = str(module)


print("\nBefore:", before)
print("\nAfter: ", after)

This prints

Before: %"res" = select nsz i1 false, double %".1", double %".2"
After:  %"res" = select nsz i1 false, double %".1", double %".2"

But should print

Before: %"res" = select nsz i1 false, double %".1", double %".2"
After:  %"res" = select fast i1 false, double %".1", double %".2"

This is also due to the string caching and makes for really hard to find bugs (took me 2h of head scratching). Numba's fastmath pass seems to work correctly, presumably because there's no printing before the pass, but that seems really brittle. I'll give the #661 PR a shot, maybe I can get it updated & merged.

So as I can see it right now there's problems with:

• [ ] Phi instructions
• [ ] call instructions that have their callee modified replace_all_calls
• [ ] Anything that's using AttributeSet which can be modified without invalidating the parent string cache:
  • [ ] call Fastmath, call CallInstrAttributes
  • [ ] function ArgumentAttributes, function FunctionAttributes
• [ ] Float math instructions and their fastmath flags (stored in .flags).

[siboehm]

(running note) I'm trying to come up with a way to fix the fast-math flag problem. It seems hard to do without breaking changes.

Current ideas:

1. For float math instructions: Make flags a tuple (non-modifable).

• Add a setter, which clears the string cache.
• Problem: This is a breaking change, since e.g. numba fastmath pass does flags.append(<new flag>) which would now start failing.

2. For the AttributeSet stuff: we could give AttributeSet its owndescr method, invalidate that when .add() is called and overload the parents's __str__ cache checking to check their child attribute's cache for validity. This would not be a breaking change, but pretty awkward in the code.