circt icon indicating copy to clipboard operation
circt copied to clipboard

[Arc] Add basic support for assert operation.

Open elhewaty opened this issue 1 year ago • 8 comments

  • Adds basic Assertion support to ARC
  • Fixes: https://github.com/llvm/circt/issues/6810

We will implement this in two steps:

  • [ ] Support the assert operation in Arc by adding arc.assert, and make sure it generates the correct LLVM code.
  • [ ] Lower verif.assert and sv.assert.concurrent operations to Arc.

elhewaty avatar Mar 17 '24 01:03 elhewaty

Aside from the conflicts, I will resolve them later: Can you share your opinions? how to generate the CHECK tests? for basic-error.mlir : this is the error unexpected error: use of value '%in' expects different type than prior uses: 'i1' vs 'i4' if I write expected-error @below {{use of value '%in' expects different type than prior uses: 'i1' vs 'i4'}} why doesn't it work?

for the print message before the abort, I want to make something like call i32 @puts(ptr @.str) can you give some hints about converting the message to Value? I tried LLVM::createPrintStrCall, but it didn't work here.

elhewaty avatar Mar 17 '24 02:03 elhewaty

how to generate the CHECK tests?

You typically write them by hand. You could, however, run the RUN command at the start of the file in the CLI, copy-paste it to the test file, and insert the CHECK prefixes, etc to save some typing. Don't forget to double check if the output actually matches what you expect.

for basic-error.mlir : this is the error unexpected error: use of value '%in' expects different type than prior uses: 'i1' vs 'i4' if I write expected-error @below {{use of value '%in' expects different type than prior uses: 'i1' vs 'i4'}} why doesn't it work?

Did you check the location of the error and insert the expected-error directly at the line before?

for the print message before the abort, I want to make something like call i32 @puts(ptr @.str) can you give some hints about converting the message to Value? I tried LLVM::createPrintStrCall, but it didn't work here.

Can you give more detail about why createPrintStrCall didn't work? The rough procedure is: create a global constant for the string, insert the puts function declaration, insert a 'addressof' to get a pointer to the global, pass its result to a call to puts. You could take a look at the lowering pattern for arc.sim.emit for an example (it uses printf instead though).

maerhart avatar Mar 17 '24 07:03 maerhart

how to generate the CHECK tests?

There's a bit of an art to this. You're trying to balance:

  1. Exactness/terseness of the test. Try to keep the test simple and only check what the test needs to test not unrelated features or those tested elsewhere.
  2. Robustness of the test. Ideally, the test should only fail for patches which actually cause the test to fail, not due to name changes, SSA value changes, or later IR additions. This is not possible to guarantee always. That's fine.
  3. Readability of the test. Hopefully, someone reading the test (or your future self) will understand what it is trying to test. Striving for (1) helps here. Comments may be necessary to explain what is going on.

Generally, your tests look good!

Usually the only thing to avoid is exact copy-pasting of output. As Martin suggests, this can be a great starting point. Sometimes it is a useful exercise to intentionally write the test from scratch to make sure you aren't inadvertently relying on incorrect output.

seldridge avatar Mar 29 '24 03:03 seldridge

I think we need to implement like llvm update_test_checks.py for circt-opt.

elhewaty avatar Mar 29 '24 03:03 elhewaty

@fabianschuiki, @maerhart ping :bell: !!

elhewaty avatar Mar 30 '24 13:03 elhewaty

This operation looks a lot like the one proposed in #6982 We should discuss with @fabianschuiki if the TableGen declaration should be moved to the Verif dialect, or if it makes sense to have two almost identical operations in both dialects.

maerhart avatar May 07 '24 07:05 maerhart

@maerhart I don't know what's wrong with the checker! I run clang-format on my local repo and I don't get any errors!!

elhewaty avatar May 14 '24 11:05 elhewaty

In the github actions summary you can download a patch it produced.

Here it is:

diff --git a/lib/Conversion/ArcToLLVM/LowerArcToLLVM.cpp b/lib/Conversion/ArcToLLVM/LowerArcToLLVM.cpp
index 120a140..7e06ed8 100644
--- a/lib/Conversion/ArcToLLVM/LowerArcToLLVM.cpp
+++ b/lib/Conversion/ArcToLLVM/LowerArcToLLVM.cpp
@@ -185,8 +185,7 @@ struct AssertOpLowering : public OpConversionPattern<arc::AssertOp> {
                                            rewriter.getBoolAttr(true)));
 
     rewriter.replaceOpWithNewOp<scf::IfOp>(
-        op, negateCondition,
-        [&](OpBuilder &builder, Location loc) {
+        op, negateCondition, [&](OpBuilder &builder, Location loc) {
           // If the condition doesn't hold then print the message and abort
           auto message = op.getMsg().value_or("");
           auto module = op->getParentOfType<ModuleOp>();

maerhart avatar May 14 '24 11:05 maerhart