This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][CodeGen] Combine SDAG and global isel tests for intersect ray intrinsics.
AcceptedPublic

Authored by kosarev on Jan 23 2023, 8:58 AM.

Details

Diff Detail

Event Timeline

kosarev created this revision.Jan 23 2023, 8:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 8:58 AM
kosarev requested review of this revision.Jan 23 2023, 8:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 8:58 AM
foad added a comment.Jan 23 2023, 9:06 AM

I see we already have test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.intersect_ray.ll. Is that redundant now? (Generally I think we should combine sdag/gisel codegen tests in a single file, as you have done here, rather than having separate files.)

kosarev updated this revision to Diff 495501.Feb 7 2023, 6:11 AM

Remove the tests duplicated in GlobalISel/llvm.amdgcn.intersect_ray.ll .

kosarev retitled this revision from [AMDGPU][CodeGen] Add global isel tests for intersect ray intrinsics. to [AMDGPU][CodeGen] Combine SDAG and global isel tests for intersect ray intrinsics..Feb 7 2023, 6:12 AM

The only differences between the tests discovered are the use of inreg and flattened arguments instead of vector ones in SDAG tests. I assumed that's what we prefer for the combined tests.

foad added a comment.Feb 8 2023, 4:07 AM

The only differences between the tests discovered are the use of inreg

Generally we would not bother with inreg except for instruction operands that have to go in SGPRs. But it doesn't really matter. At worst, it will bloat the output with some extra SGPR-to-VGPR copies.

and flattened arguments instead of vector ones in SDAG tests. I assumed that's what we prefer for the combined tests.

Again it doesn't really matter. Personally I would use vector arguments because it keeps the IR nice and short.

arsenm added a comment.Feb 8 2023, 4:18 AM

The only differences between the tests discovered are the use of inreg

Generally we would not bother with inreg except for instruction operands that have to go in SGPRs. But it doesn't really matter. At worst, it will bloat the output with some extra SGPR-to-VGPR copies.

and flattened arguments instead of vector ones in SDAG tests. I assumed that's what we prefer for the combined tests.

Again it doesn't really matter. Personally I would use vector arguments because it keeps the IR nice and short.

No no, all the permutations should be tested. This is not a cosmetic choice, we should be testing these are handled correctly

kosarev added a comment.EditedFeb 16 2023, 11:33 AM

No no, all the permutations should be tested. This is not a cosmetic choice, we should be testing these are handled correctly

That's what the comments say, yes. But then if vector versions have only been used to make sure the register operands are where the values actually land, maybe we can test that just by adding some extra arguments to flattened versions and thus make accidentally correct output sufficiently unlikely?

Tests like this don't feel to be the right place to test codegen'ing of moves preparing the operands and cluttering tests checking them repeatedly for various kinds of calls probably doesn't make much sense as well?

foad accepted this revision.Feb 23 2023, 2:21 AM

Looks fine to me, thanks.

This revision is now accepted and ready to land.Feb 23 2023, 2:21 AM

@arsenm Matt, are you fine with this as-is? If not, would adding more parameters as described in https://reviews.llvm.org/D142375#4132791 be enough?

No no, all the permutations should be tested. This is not a cosmetic choice, we should be testing these are handled correctly

That's what the comments say, yes. But then if vector versions have only been used to make sure the register operands are where the values actually land, maybe we can test that just by adding some extra arguments > to flattened versions and thus make accidentally correct output sufficiently unlikely?

I don't really follow what this is saying. One reason for the split would have been that SelectionDAG is broken in the trickier cases that require waterfall loops

Tests like this don't feel to be the right place to test codegen'ing of moves preparing the operands and cluttering tests checking them repeatedly for various kinds of calls probably doesn't make much sense as well?

It's making sure they codegen for any inputs, which would also cover the waterfall cases?

I still think all the S/V permutations should be covered in these intrinsic tests. The benefit of merging the tests is not worth losing coverage to work around the DAG not working