[hw] Add hw.donttouch operation

[seldridge]

Add a hw.donttouch operation which is an inner symbol user that
indicates that the inner symbol should not be optimized, removed, or
otherwise touched by the compiler.

This is added to fill a hole where unused inner symbols are being used to
mean "don't touch". The lack of a user means that all inner symbols are
effectively "don't touch" when only certain ones should be. Concretely,
this is being added as a step towards better optimization of read probes.

[dtzSiFive]

As-is, this has no results, symbol, and is not side-effecting.

I think this should be marked as having side-effects to avoid being dropped. It should be (if we care?) valid to CSE these.

I'd expect these all at top-level of an IRN -- should we add that restriction/expectation ? That is, these are expected/supported as siblings to hw.module's not within them, that sort of thing?
(if this is intended to be more flexible, nevermind...)

[seldridge]

I tried a couple of different approaches here. However, I eventually realized that MLIR will behave conservatively and do nothing if the op doesn't implement the memory effect interface: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Interfaces/SideEffectInterfaces.cpp#L325 It seems sufficient to do nothing here to cause CSE to do no harm.

The other alternatives are to mark this as a MemWrite which will have the same blocking effect. Both MemRead and MemAlloc don't block CSE/DCE. (I didn't check MemAlloc, only read the code for that one.) The real problem is I couldn't get a combination of side effecting traits which will block DCE, but not CSE. For effects, those seem to almost go hand-in-hand.

[teqdruid]

I haven't looked over this PR, but this feels like it should be a dialect attribute. Those have problems related to passes not retaining them though.

…

On Mon, Jun 9, 2025, 2:10 PM Will Dietz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/circt/Dialect/HW/HWStructure.td <#8547 (comment)>: > @@ -653,6 +654,22 @@ def HierPathOp : HWOp<"hierpath", }]; } +def DontTouchOp : HWOp<"donttouch"> { + let summary = "A user of an inner ref that blocks removal"; + let description = [{ + An operation that marks an InnerRef as having external users. The operation + with the InnerRef should not be optimized in any way and may be used + externally by name. + + This operation roughly implements the notion of "don't touch" that many + Verilog tools understand. + }]; + let arguments = (ins InnerRefAttr:$ref); + let assemblyFormat = [{ + $ref attr-dict + }]; implement InnerRefUserOpInterface, for verification the reference exists/is valid . ------------------------------ In include/circt/Dialect/HW/HWStructure.td <#8547 (comment)>: > @@ -653,6 +654,22 @@ def HierPathOp : HWOp<"hierpath", }]; } +def DontTouchOp : HWOp<"donttouch"> { As-is, this has no results, symbol, and is not side-effecting. I think this should be marked as having side-effects to avoid being dropped. It should be (if we care?) valid to CSE these. I'd expect these all at top-level of an IRN -- should we add that restriction/expectation ? That is, these are expected/supported as siblings to hw.module's not within them, that sort of thing? (if this is intended to be more flexible, nevermind...) — Reply to this email directly, view it on GitHub <#8547 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALNXYAA7UALXCN7G2J2Q5L3CXE2VAVCNFSM6AAAAAB65RTGE6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSMJQHA4TKNRUG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[uenoku]

Is there difference between public hierpath op which points to the an inner symbol?

[seldridge]

@teqdruid wrote:

I haven't looked over this PR, but this feels like it should be a dialect
attribute. Those have problems related to passes not retaining them though.

That was roughly how FIRRTL handled it and we could go in this direction. However, it's not currently how this is implemented. Currently, hardware donttouch is implemented with a dangling inner symbol. This could be viewed as a patch of the existing design point.

@uenoku wrote:

Is there difference between public hierpath op which points to the an inner symbol?

In terms of current effect, no. In terms of what a public hierpath op means, I'm not sure. I think this would have basically the same meaning/restrictions. I was. avoiding the public hierpath op as it seemed better not to abuse that to also mean "don't touch".

[seldridge]

We discussed this internally and the rough consensus was that an attribute is definitely better. This is a relatively annoying lift, though. Alternatively, changing this to a single, variadic operation was viewed as better given that this is a relatively static thing.

[teqdruid]

We discussed this internally and the rough consensus was that an attribute is definitely better. This is a relatively annoying lift, though.

What, AI isn't capable of making the change?

[seldridge]

What, AI isn't capable of making the change?

It's more the implications of it. Currently unused inner symbols are being used for this and that is (assumedly) blocking optimizations (as it is having this effect). This then needs a new attribute that all optimizations, passes, etc. understand the meaning of it. This is how we handle this in FIRRTL dialect, it's just pretty invasive.

I think we went the inner symbol option originally as it doesn't have this problem. If something has an inner symbol, then it can't be deleted unless you analyze users (and that should be the job of inner symbol DCE, not your folder/pass). However, inner symbol DCE is only done in FIRRTL (primarily because verification of inner symbols isn't yet automatically supported as we don't have the hw.design container op).

[teqdruid]

I understand how invasive a change this could end up being. I was mostly joking... Current AI models don't work terribly well on our code base. But I've heard stories about AI being let loose on complete code bases and doing really well. So as of next month, all changes to CIRCT will be trivial, right?

[seldridge]

Only if you can figure out how to enable AI code review on the project. 😉

[teqdruid]

#8486

Does not everyone have this option?