Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HWToLLVM][Arc] Add out-of-bounds handler for array accesses #6956

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fzi-hielscher
Copy link
Contributor

Prevent hw.array_get operations from exceeding their memory bounds after lowering to LLVM. This adds a dynamic check to the index operand (if required) which can invoke a handler function to fix-up the situation or just terminate.

A stub handler quietly redirecting the access to the array's first element is added directly to the IR. The handler can be overridden at link time to enable user-defined behavior. A "default custom" handler is added to the arcilator runtime header, which emits an error message and terminates the simulation.

I'd be happy to get some feedback on the general approach first, so I'll mark this as draft. If we can agree on this approach, I'll add tests and a similar handler for hw.array_slice.

Fixes #6949

@fzi-hielscher fzi-hielscher added the Arc Involving the `arc` dialect label Apr 26, 2024
@maerhart
Copy link
Member

maerhart commented May 7, 2024

Sorry for the late review!

This approach allows us to override the OOB access behavior at a very late stage in the lowering pipeline. I wonder if we necessarily need this or whether it might be better to leave it more restricted.

I assume we want to define OOB accesses the same way as division by zero. This means we get an undefined value. Optimization passes can use these semantics for optimizations as long as the transformations are refinements, e.g., always return 0. The same restriction has to hold for the callback functions provided here, otherwise, the original language semantics might not be respected, making it easy for the user to accidentally break things. Furthermore, the user cannot assume that their callback function is executed for all OOB accesses originally in the code because some of them might already have been optimized away at this stage (e.g., making it useless for collecting metrics/coverage).

If a language spec leaves some room for interpretation on what should happen in such a case during simulation, I feel like it might be better to let the user influence this at some point during the frontend language lowering pipeline already.

Do you have particular use cases in mind that require this generality?

@fzi-hielscher
Copy link
Contributor Author

Sorry for the late review!

No worries! Thanks a lot for your feedback.

This approach allows us to override the OOB access behavior at a very late stage in the lowering pipeline. I wonder if we necessarily need this or whether it might be better to leave it more restricted.

I would not argue that we need this, but in general I like to err on the side of flexibility if it comes at a low cost. The combination of weak linking and the branch weight should hopefully keep the performance impact at a minimum, compared to an inlined handler, while still giving users the option to hook into it. At the same time it is effectively transparent for users who do not define a custom handler.

I assume we want to define OOB accesses the same way as division by zero. This means we get an undefined value. Optimization passes can use these semantics for optimizations as long as the transformations are refinements, e.g., always return 0. The same restriction has to hold for the callback functions provided here, otherwise, the original language semantics might not be respected, making it easy for the user to accidentally break things.

Usually I am the last person to argue against following language semantics as specified. And I'd insist that this should be the default. But I guess my mind has been (pun intended) poisoned by certain "unfortunate" language design decisions (e.g., implicit coercion of X to 0 in Verilog) that I see value in having the option to override them. Naturally, this should not occur by accident, but do you really see the risk of accidentally linking a "wrong" handler?

Furthermore, the user cannot assume that their callback function is executed for all OOB accesses originally in the code because some of them might already have been optimized away at this stage (e.g., making it useless for collecting metrics/coverage).

This is a good point I have not really considered so far. But I do not necessarily see it as a deal breaker. I guess the goal from my perspective would be to have the Arcilator simulation resemble the amount of non-determinism of a post ExportVerilog simulation given the same IR as closely as possible. From that point I would generally assume that any refinement of a non-deterministic to a deterministic expression would affect both simulations in the same way. It also touches on the high-level question of to what extent we even want to refine non-determinism. Of course, for verification non-determinism stinks and the more we can get rid of, the better. But I think we should also consider that any refinement to a specific value might constraint a synthesis backend in an unfavorable way.
In essence, I would not consider the handler as a "guaranteed to be invoked if an OOB access occurs by the input language semantics" function. Instead, its invocation shows that it can happen and the user might want to know about this.

If a language spec leaves some room for interpretation on what should happen in such a case during simulation, I feel like it might be better to let the user influence this at some point during the frontend language lowering pipeline already.

I have mixed feelings regarding what decisions should be made at simulation runtime vs. what should be codified by the frontend into the core IR. I understand the desire of having the IR as single source of truth. But at the same time I consider simulation to be interactive to an extent that cannot possibly be covered by frontend knobs. For OOB accesses specifically, the SystemVerilog LRM states that they may cause a warning message to be issued. Right now, if I'm not mistaken, we do not have any option to specify emission of a runtime warning in the IR, at least not in a way Arcilator can handle. But let's assume we had one. What if I want to have those warnings, but suppress them during a specific time frame (e.g. while the DUT is in reset)? What if I want to ignore them for a specific array, but for others terminate the simulation and create a dump of the state? Technically we could encode all that in the IR, but I'm having a hard time seeing that as a realistic option. So, in the end the frontend would have to install its own hook for a user defined handler, wouldn't it?

Do you have particular use cases in mind that require this generality?

Nothing specific, no. But if we should go the route of division by zero and specify that an OOB access is an arbitrary combinatorial function on the array's contents and the index, IMHO it would be nice to give users the option to exercise this freedom and see if it breaks the design. E.g., return the value at a randomly selected index, or the MD5 sum of the array's contents or whatnot. More generally an (for lack of a better term) "ABI" for interactions between the C/C++ runtime environment and the model's eval function, giving an easy way of hooking into the simulation, is definitely on the list of things I'm interested in. And I feel there are few things more cumbersome than having to write custom MLIR passes building LLVM IR to implement mostly static functionality. 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arc Involving the `arc` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HWToLLVM] ArrayGet lowering produces out-of-bounds memory access
2 participants