[Seq] Add cast operation to immutable type

[uenoku]

This adds a cast operation from normal type to immutable type.

[fabianschuiki]

I'm wondering if you have a concrete use case for this in mind. Otherwise, we could just try to not have the ToImmutableOp initially and only add it once we clearly need it. WDYT?

[uenoku]

Yes, I added ToImmutableOp to represent firreg randomization since async firreg uses reset in random initialization (#3000). I agree that this is very tricky and alternative solution would be (1) to allow seq.initial to take normal type operands (currently only immutable values are allowed) or (2) to impose some restriction that the input of ToImmutableOp must be a port. WDYT?

[fabianschuiki]

Ah I see, yeah that's a pretty tricky use case 🤔. It's so sad that async regs are modelled slightly wrong in Verilog because they use a posedge/negedge trigger instead of a level-based trigger. So sad 😢.

Could the semantics here be that seq.to_immutable basically gives you the initial value of something in the IR? Basically a frozen version of it at initialization time? That sounds like it would be safe. Maybe something like seq.get_initial_value or seq.freeze_initial_value?

[fzi-hielscher]

FWIW, I agree with @fabianschuiki. to_immutable is giving me strong const_cast vibes and would really undermine the semantics of !seq.immutable. A seq.get_initial_value op makes sense to me conceptually, but I'd be very careful about the naming and definition of it. To me "initial value" could refer to any of those:

1. The SSA value occurring after the initial keyword of the defining op
2. The first concrete value observable on the given SSA value during simulation
3. The concrete value observed on the given SSA value right after initialization

(1) is definitely not what we are trying to get at. In an ideal world (2) and (3) would be identical. But for SV lowering I'm not sure we can entirely avoid a static initialization order fiasco for either of these two options.

[uenoku]

seq.get_initial_value seems like a definitely better naming, thanks!

Could the semantics here be that seq.to_immutable basically gives you the initial value of something in the IR?

Yes, seq.to_immutable is meant to copy the value at initialization phase. The contract is that its input value must be valid at initialization phase (so the value is usually derived by top-level input ports).

to_immutable is giving me strong const_cast vibes

Unfortunately yes. const_cast is a good analogy for to_immutable. to_immutable imposes strong assumption that the input is correctly driven by users at initialization.

(1) The SSA value occurring after the initial keyword of the defining op
(2) The first concrete value observable on the given SSA value during simulation
(3) The concrete value observed on the given SSA value right after initialization

That's good point. Actually my intention was to sample a pre-initialization value so (2) would be closest. For combinatorial logic ops whose fan-in is all ports (2) and (3) are equal but for registers (2) and (3) are different. I hope we can reject registers being used as input of the cast op but it's not possible to detect all cases.

[fabianschuiki]

Looking through the SV spec a bit, it looks like the order in which initial procedures are executed is undefined. If we have something like the following:

hw.module @Foo() {
  %0 = seq.initial { seq.yield 42 }
  %a = seq.compreg [...] initial %0
  hw.instance "bar" @Bar(%a)
}
hw.module @Bar(%a) {
  %1 = seq.get_initial_value %a
  %b = seq.compreg [...] initial %1
}

there might be a race condition between the initializers for registers a and b. I'd expect this to get translated into the following Verilog:

module Foo;
  int a;
  initial a = 42;
  Bar bar(a);
endmodule

module Bar(input int a);
  int b;
  initial b = a;
endmodule

Depending on which initial gets executed first, register b would end up being initialized to 0 or 42.

Do you know if we can somehow guarantee that get_initial_value will always get the true initial value? If we can't, maybe we really need to mark this as an unsafe op and leave it up to the user to ensure that there are no competing initial assignments. Maybe something like seq.to_immutable_unsafe, or seq.unsafe_to_immutable, or seq.unsafe_immutable_cast to make it very clear that this op is needed for a hack where some Chisel users have a logic reset = 1 in the testbench that is correctly read by initial blocks.

[fzi-hielscher]

Do you know if we can somehow guarantee that get_initial_value will always get the true initial value?

Only if we have full control over the module hierarchy and can change module interfaces. In that case, we can explicitly sequence initialization in the submodules from a single initial in the toplevel by sending events around. I had drafted something like this to enforce a specific order on side-effecting procedure calls in the sim dialect across modules and instances. - It's horrible. But if the driving testbench is not under our control, I think we are pretty much screwed. That's what I meant with "static initialization order fiasco".

In general, it seems inadvisable to read module ports in initializers. If we have to do it anyway, we probably want some sort of ABI contract, guaranteeing that the port is fully initialized before any initial procedure is scheduled. I have been staring at LLVM function attributes for the last couple of days, so my thinking may be a little skewed. But maybe this would be a valid use case for port attributes? Basically, by adding a preinitialized attribute to the port, we promise that it can be safely used in an initial op. Otherwise we assume the initial value of module inputs to be undefined and emit a diagnostic if it is accessed anyway.

[uenoku]

Yes initial statements execution order is totally undefined.

Depending on which initial gets executed first, register b would end up being initialized to 0 or 42.

Yes this kind of use is undefined so I mentioned in the op description:

The input value must be valid at the initialization and immutable through the initialization phase and otherwise the result value is undefined. In other words time-variant values such as registers cannot be used as an input.

Do you know if we can somehow guarantee that get_initial_value will always get the true initial value?

If we perform inter-module dataflow analysis, yes I think we can guarantee that no register is used as an input of get_initial_value. But it would be hard to verify with local analysis unless we introduced a new type (!seq.valid_in_initial whatever but I generally don't want to introduce a new type for this narrow use case).

it seems inadvisable to read module ports in initializers.

Yes I agree but for async reset specifically I think it's valid use case (since async reset is usually enabled at power-on FWIW). FYI this async reset initialization is implemented in Chisel since async reset is introduced.

But maybe this would be a valid use case for port attributes?

I think types would be better fit over attributes for this kind of property. Since the use case would be only async reset , it might be sufficient to restrict the input of seq.get_initial_value to async reset value (currently we don't have async reset type in Seq but we had a plan for that #7215).

[fzi-hielscher]

I think types would be better fit over attributes for this kind of property. Since the use case would be only async reset , it might be sufficient to restrict the input of seq.get_initial_value to async reset value (currently we don't have async reset type in Seq but we had a plan for that #7215).

Yeah, but if we narrow it down to that specific use case and the only motivation is to produce that specific (somewhat unsafe) SV pattern, I'd personally rather not introduce a dedicated op for it. Assuming we add a !seq.async_reset type, couldn't we then just change the lowering of compreg to match the current behavior of an async reset firreg if the reset value has that type?

[uenoku]

Sorry for delayed response. I pursed the direction that avoids this kind of use of reset but seems impossible to deprecate this behavior in Chisel, and even more it's fairly common to use non-stable reset value in the initial phase. Adding special handling to compreg SV lowering would work but I'm slightly concerned that we have to lower compreg to semantically different representation for SV and Arc.

[uenoku]

I separated immutable cast for now since it will especially cause some issue in Arc initialization. For SV I'll add if(reset) when lowering async compreg.