search

nod-ai / iree-amd-aie

IREE plugin repository for the AMD AIE accelerator
Apache License 2.0
69 stars 30 forks source link

Delete obviated tests #799

Closed makslevental closed 1 month ago

makslevental commented 1 month ago

This functionality is now better tested in compiler/plugins/target/AMD-AIE/iree-amd-aie/Target/test and runtime/src/iree-amd-aie/aie_runtime/test.

makslevental commented 1 month ago

Hey @newling can I delete "smoke test" as well? I think we have a good handle on those flags now?

newling commented 1 month ago

This test was initially created check that flags like aie2xclbin-print-ir-before-all were working, but those flags are now basically not relevant so I'm ok with the test being removed (although it seems to be testing more now than it did when I initially created it).

Hey @newling can I delete "smoke test" as well? I think we have a good handle on those flags now?

The test is there to prevent drift, to make sure those flags keep working. Why do you want to delete it?

makslevental commented 1 month ago

The test is there to prevent drift, to make sure those flags keep working. Why do you want to delete it?

Because I'm assuming that now we understand the caveats of how argparser parses flags so why keep a test around that just tests that understanding? It's not like the implementation of that parsing is going to drift?

EDIT: and it's not free - every time we change the flags/args to cpu_comparison/run.py we have to update the smoke test and since the logic is different from the other tests it takes some amount of squinting to change.

newling commented 1 month ago

The test is there to prevent drift, to make sure those flags keep working. Why do you want to delete it?

Because I'm assuming that now we understand the caveats of how argparser parses flags so why keep a test around that just tests that understanding? It's not like the implementation of that parsing is going to drift?

EDIT: and it's not free - every time we change the flags/args to cpu_comparison/run.py we have to update the smoke test and since the logic is different from the other tests it takes some amount of squinting to change.

Fair points, you can remove it. I just hope the flag --do-not-run-aie keeps working, that's the one I care most about in that test.