This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Starting fixing issues that prevent us from testing vXi64 intrinsics on RV32.
ClosedPublic

Authored by craig.topper on Mar 3 2021, 4:07 PM.

Details

Summary

Currently we crash in type legalization any time an intrinsic
uses a scalar i64 on RV32.

This patch adds support for type legalizing this to prevent
crashing. I don't promise that it uses the best possible codegen
just that it is functional.

This first version handles 3 cases. vmv.v.x intrinsic, vmv.s.x
intrinsic and intrinsics that take a scalar input, splat it and
then do some operation.

For vmv.v.x we'll either rely on hardware sign extension for
constants or we'll convert it to multiple splats and bit
manipulation.

For vmv.s.x we use a really unoptimal sequence inspired by what
we do for an INSERT_VECTOR_ELT.

For the third case we'll either try to use the .vi form for
constants or convert to a complicated splat and bitmanip and use
the .vv form of the operation.

I've renamed the ExtendOperand field to SplatOperand now use it
specifically for the third case. The first two cases are handled
by custom lowering specifically for those intrinsics.

I haven't updated all tests yet, but I tried to cover a subset
that includes single-width, widening, and narrowing.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 3 2021, 4:07 PM
craig.topper requested review of this revision.Mar 3 2021, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 4:07 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

Fix a typo and clang-format.

craig.topper added inline comments.Mar 3 2021, 4:12 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2453

I was thinking maybe we just need two slide1ups using SEW=32 with VL set to 2 so that we don't slide anything but the scalars we're inserting.

llvm/lib/Target/RISCV/RISCVISelLowering.h
102

I think @frasercrmck has a patch that adds VMV_S_XF_VL which I think I can migrate to once its committed.

craig.topper retitled this revision from [RISCV] Starting fixing issues the prevent us from testing vXi64 intrinsics on RV32. to [RISCV] Starting fixing issues that prevent us from testing vXi64 intrinsics on RV32..Mar 4 2021, 8:09 AM
frasercrmck added inline comments.Mar 4 2021, 8:22 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2453

Crafty; I like it. Doing that later along with INSERT_VECTOR_ELT would be my preferred way to go.

llvm/lib/Target/RISCV/RISCVISelLowering.h
102

Yeah you should be good to go with that now.

craig.topper added inline comments.Mar 4 2021, 9:29 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2453

I was also wondering if we could do something like this for the splat

vmv.v.x vX, hi // using SEW=64
vsll.vx vX, vX, /*32*/ // clear the lower 32 bits like we're doing now.
vsetvli e32 // same vl with half the lmul of the 64 bit type.
vaddu.wx vX, vX, lo // should zero extend the lo value to 64 bits by zero extending. Since we cleared the lower 32 bits above this is equivalent to OR.
vsetvli e64 // mask to original sew/lmul

The nice advantage it has is that it can be done in one physical register or physical register group. The current sequence requires two.

Remove custom node now that Fraser's patch is in

frasercrmck added inline comments.Mar 8 2021, 2:25 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2453

Sounds like that'd work, yep.

Off the top of my head, could you not also extend the INSERT_VECTOR_ELT sequence with a vrgather.vi vd, vs2, 0 to splat the first element? Perhaps fewer instructions but you'd still that second register due to the non-overlap constraint. I'm not sure which is better: perhaps it's situational.

craig.topper added inline comments.Mar 8 2021, 10:43 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2453

Off the top of my head, could you not also extend the INSERT_VECTOR_ELT sequence with a vrgather.vi vd, vs2, 0 to splat the first element? Perhaps fewer instructions but you'd still that second register due to the non-overlap constraint. I'm not sure which is better: perhaps it's situational.

I'm not following. INSERT_ELEMENT has a scalar register input input. How did it get to element 0 for vrgather.vi vd, vs2, 0?

frasercrmck added inline comments.Mar 9 2021, 1:33 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2453

Ah sorry I might have skipped a step. I was proposing extending the two slide1up sequence you proposed earlier to get it from element zero to all elements.

craig.topper added inline comments.Mar 9 2021, 9:42 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2453

Ok I get it now. When I wrote my slide1up suggestion I was only thinking about this case where element 0 was the final location.

frasercrmck accepted this revision.Mar 10 2021, 2:16 AM

Anyway, LGTM.

This revision is now accepted and ready to land.Mar 10 2021, 2:16 AM
This revision was landed with ongoing or failed builds.Mar 10 2021, 9:45 AM
This revision was automatically updated to reflect the committed changes.