riscv-software-src / riscv-tests

Other
869 stars 449 forks source link

Missing dependency from 32-bit tests to 64-bit .S file in isa/Makefile #266

Open takahirox opened 4 years ago

takahirox commented 4 years ago

Problem

32-bit ISA unit test has dependency to 64-bit *.S files because rv32*/%.S includes 64-bit %.S file, for example rv32ui/add.S includes rv64ui/add.S.

But isa/Makefile seems to miss this dependency so if I update 64-bit *.S file make rebuilds only 64-bit test and doesn't 32-bit test. For example if I update rv64ui/add.S and then run make it rebuilds only rv64ui-p/v-add and doesn't do rv32ui-p/v-add although it should.

Root issue

The root issue in isa/Makefile seems here.

# # is comment by takahirox here
# $(1) is rv32?? or rv64?? 
# for p test
$$($(1)_p_tests): $(1)-p-%: $(1)/%.S

# for v test
$$($(1)_v_tests): $(1)-v-%: $(1)/%.S

It doesn't take care 64-bit .S file from 32-bit mode test.

Solution idea

I'm trying to make PR to resolve this issue by replacing the above with

# for p test
$$($(1)_p_tests): $(1)-p-%: $(1)/%.S $(subst rv32,rv64,$(1))/%.S

# for v test
$$($(1)_v_tests): $(1)-v-%: $(1)/%.S $(subst rv32,rv64,$(1))/%.S

$(subst rv32,rv64,$(1))/%.S replaces, for example rv32ui/%.S with rv64ui/%.S, so it can specify dependency from 32-bit test to 64-bit .S files. If $(1) is rv64?? it just adds duplicate strings with $(1)/%.S so no effect.

Question

But one problem here. shamt.S file is only in rv32mi/, not in rv64mi/. So the above solution fails due to no rv64mi/shamt.S file for rv32mi-p-shamt.

As workaround, I'm thinking to add dummy shamt.S in rv64mi/. If we do, the above solution works. Does it sound good?

Or, I'm not familiar with Makefile so please let me know if there is a proper way to solve only in Makefile side, for example skipping dependency to non-existent file.

Plus, I expect all rv32*/%.S, except for rv32mi/shamt.S, include rv64*/%.S. Let me know if my expectation is wrong.

sermazz commented 3 years ago

Stumbled upon this issue too!