[ImportVerilog]Support real math functions.

[hailongSun2000]

No description provided.

[fabianschuiki]

This looks great! Thanks for adding all of this. Just a minor comment about first checking that the syscall has exactly 1 argument. Besides that, this looks good to go!

[fabianschuiki]

We need to check whether args.size() == 1 first. Maybe you could wrap the convertRvalueExpression and your entire StringSwitch in an if (args.size() == 1) { ... }? In the future, when we support different arities of system tasks/calls, we can expand this to call into some special handling functions like convertSystemCall1, convertSystemCall2, convertSystemCallVariadic, and similar. Not needed now though, since all we support are unary/arity-1 system calls that have a single argument.

[hailongSun2000]

I changed the original realMathFunc() into convertSystemCallArity1(value) and moved it out of the if block. In the future, we can add convertSystemCallArity2(x, y) or convertSystemCallArityN() without args. What do @fabianschuiki think about this tweak 🤔?

[fabianschuiki]

Nice! How about turning this into a proper function instead of a large inline lambda?

[fabianschuiki]

I think you still need to move all of this into a if (args.size() == 1). Further up where you call context.convertRvalueExpression(*args[0]), this call will crash if args.size() == 0.

I'd suggest moving the convertSystemCallArity1 into a proper function and doing something like this:

if (args.size() == 1) {
  auto value = context.convertRvalueExpression(*args[0]);
  if (!value)
    return {};
  auto result = convertSystemCallArity1(value);
  if (failed(result))
    return {};
  if (*result)
    return *result;
}

One thing you need to do in convertSystemCallArity1 is you need to make it clear from the return value whether:

1. The conversion was successful and you return the result.
2. The conversion failed due to an error, e.g. that convertToSimpleBitVector fails.
3. The system call is unknown.

I'd change the return type to FailureOr<Value>, and then doing the following:

1. Return result of the created op.
2. Return failure() due to an error.
3. Return Value{} for unsupported system calls.

[hailongSun2000]

You're right! Regarding if(args.size() == 1), I attempted the $random() and it will crash.

[hailongSun2000]

I'd change the return type to FailureOr, and then doing the following:

1. Return result of the created op.
2. Return failure() due to an error.
3. Return Value{} for unsupported system calls.

I have a question about why we'll change the return type to FailureOr<Value>. I notice FailureOr<Value> convertFormatString, but this function is used in Statement.cpp. However, convertToSimpleBitVector returns Value, even if failed conversion, it just returns Value{} with an error message and not LogicalResult, so if(failed(result)) is never to be triggered.

[fabianschuiki]

You're definitely right! The difference is that convertToSimpleBitVector only needs to distinguish two cases:

1. successful conversion (a non-null Value is returned)
2. failed conversion (a null Value is returned)

However, convertSystemCallArity1 needs to distinguish three cases:

1. successful conversion (a non-null Value is returned)
2. function name recognized, but conversion failed (failure() is returned)
3. function name not recognized (a null Value is returned)

The FailureOr<Value> allows you to distinguish a failure from the case where the function didn't recognize the function name.

[hailongSun2000]

Thanks for your detailed explanation!

[hailongSun2000]

Maybe this is reasonable? First time to use std::function<FailureOr<Value>()>. It just looks correct 😂.

[fabianschuiki]

Nice! 😃

[hailongSun2000]

Greatly appreciate your guidance!

[hailongSun2000]

Borrow from #8020 .