Mismatch between hex and decimal values in the comments section of generated .S file

[anku-anand]

In the attached clmul-01.S file,

inst_1:
// rs1 == rs2 == rd, rs1==x29, rs2==x29, rd==x29, rs2_val == 9223372036854775807, rs1_val == 18446744073709550591
// opcode: clmul ; op1:x29; op2:x29; dest:x29; op1val:0xfffffffffffffbff; op2val:0xfffffffffffffbff
TEST_PKRR_OP(clmul, x29, x29, x29, 0x0000000000000000, 0xfffffffffffffbff, 0xfffffffffffffbff, x29, x1, 8, x2)

rs2_val and its hex equivalent in op2val are not same

Similarly in
inst_3:
// rs1 == rs2 != rd, rs1==x27, rs2==x27, rd==x30, rs2_val == 16140901064495857663,
// opcode: clmul ; op1:x27; op2:x27; dest:x30; op1val:0xfffffffffffffbff; op2val:0xfffffffffffffbff
TEST_PKRR_OP(clmul, x30, x27, x27, 0x0000000000000000, 0xfffffffffffffbff, 0xfffffffffffffbff, x27, x1, 24, x2)
rs2_val and its hex equivalent in op2val are not same

###########
riscv_ctg --version
RISC-V Compliance Test Generator, version 0.6.3

python3 --version
Python 3.6.9
clmul.cgf.txt
clmul-01.S.txt

[allenjbaum]

To be clear: there are 2 comments, and it's the first comment that is clearly wrong, since rs1==rs2, yet rs1val!=rs2val
This doesn't affect tests. I don't know the source of first comment (e.g. is it coming from the coverpoint file?) so hard to know if has any practical affect.
As a nit (actually, 2 nits: ):

• I would prefer that all operands be listed in hex, because it is impossible to determine by eye whether a large decimal number is equivalent to anything, while it's usually quite easy to resolve hex numbers.
• It would be nice if the comments were consistent about ordering (rs1 usually comes before rs2, but is reversed in that first comment).

[ptprasanna]

Would be nice if the sign is also represented in hex rather explicit sign as below for the hex values.

inst_5:// rs1==x24, rd==f11, rs1_val == -1227077728 and rm_val == 0
// opcode: fcvt.s.l ; op1:x24; dest:f11; op1val:-0x4923b860; valaddr_reg:x16; val_offset:40; rmval:0x0; correctval:0; testreg:x18
TEST_FPIO_OP(fcvt.s.l, f11, x24, 0x0, 0, x16, 40, x17, x15, 0, x18)

inst_6:// rs1==x7, rd==f7, rs1_val == 1227077728 and rm_val == 4
// opcode: fcvt.s.l ; op1:x7; dest:f7; op1val:0x4923b860; valaddr_reg:x16; val_offset:48; rmval:0x4; correctval:0; testreg:x18
TEST_FPIO_OP(fcvt.s.l, f7, x7, 0x4, 0, x16, 48, x17, x15, 16, x18)

[allenjbaum]

My preference is that hex values be raw hex with the exception of branch/jump and load/store offset values.
Anything that's absolute should be raw hex without a minus sign if the SMB is a 1.

[pawks]

The first line in the comments is the set of coverpoints which caused a particular test case to be generated. These are lines directly from the cgf. The second line is the key to identifying the values in the macro arguments, sort of a hint to understand which is which. Now the values don't match because of the rs1==rs2 in both of these cases. This is because the operands and the values are generated independent of each other and then combined together to write out the test cases. But when 2 or more registers are the same, there is a possibility that a value combination coverpoint might not be satisfied(due to value overwrites in case of using the same regsiter). In such a scenario, the value of the operands is equated out. This is the reason you see the mismatch in the values in both the comments. But this does not mean that the coverpoint for rs2_val is not hit. CTG replicate the value combination once again towards the end of the test. The comment is right with respect to the values provided to the macro instance and accurate. The first comment should always be taken with a grain of salt. I'm not sure whether sanitising the first comment is a low hanging fruit which can be easily implemented.

[allenjbaum]

OK, I get it. The first comment is effectively superseded by the second, because of the rs1==rs2 condition.
So the test parameters (registers used, and values in the registers) are generated and commented, but when actually applied, in the tests, the rs2_val that was generated wasn't used.
That's more confusing than misleading, but doesn't affect test results or quality.
That could/should be filtered out at some point, but I would rate it at low priority

[anku-anand]

In riscv_ctg/generator.py
###########
def instr(self, op=None, val=None):
cond_str = ''
if op:
cond_str += op[-1]+', '
if val:
cond_str += val[-1]
########
if val:
cond_str += val[-1]
above two lines adds the decimal values of rs2_val in the comments. This is redundant information.
Hex values of the same are already added to the comments in .S file.
Commenting out the above two lines will avoid redundant information and also this issue.

[pawks]

That will remove the values for cases where rs1_val and rs2_val are indeed different. Fixing this infact depends on multiple conditions such as whether rs1 is loaded first or rs2 is inside the macro and whether both the vals are equal. IMO its fine the way it is, since it indicates correctly the values passed to the macro. The observed values are different due to the program flow and that is how it should be. Very few instances of the macros generated will encounter this problem(i.e rs1 and rs2 are never the same register unless a coverpoint explicitly specifies it). This is more of a nice to have but needs additional information about the macros to figure out the correct value to be appended to the comments.