This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Update RVV shift intrinsic tests to use XLEN bit as shift amount.
ClosedPublic

Authored by khchen on Mar 12 2021, 6:07 AM.

Details

Summary

Fix the unexpected of using op1's element type as shift amount type.

Diff Detail

Event Timeline

khchen created this revision.Mar 12 2021, 6:07 AM
khchen requested review of this revision.Mar 12 2021, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 6:07 AM

I can see that the shift amount should be XLen, but is there a crash that this fixes? Or is it that it shouldn't be possible to generate these intrinsics in the first place? I'm wondering what (if anything) can be put in place to stop this from happening again.

I can see that the shift amount should be XLen, but is there a crash that this fixes? Or is it that it shouldn't be possible to generate these intrinsics in the first place? I'm wondering what (if anything) can be put in place to stop this from happening again.

I think it's shouldn't be generated and we noted them when we start working on supporting rvv intrinsics.

Maybe we could define the explicitly type for vector/vector and vector/scalar in the IR intrinsic, it would make intrinsics more strongly typed.
But personally I prefer to keep current intrinsic design...

frasercrmck accepted this revision.Mar 17 2021, 7:53 AM

I think it's shouldn't be generated and we noted them when we start working on supporting rvv intrinsics.

Maybe we could define the explicitly type for vector/vector and vector/scalar in the IR intrinsic, it would make intrinsics more strongly typed.
But personally I prefer to keep current intrinsic design...

Yeah fair enough, I see the issue. I don't propose we change the intrinsic design. I was also generating intrinsics at some point and was having difficulty determining which types to provide, as it tends to just crash when you get it wrong.

Ideally we'd have a check in the backend that the types are as we expect. Either explicit (asserts) or implicit (it doesn't select).

But all of that is increasing the scope of this patch, and this patch is already slowing down my browser, so let's get it in sooner rather than later :)

This revision is now accepted and ready to land.Mar 17 2021, 7:53 AM