This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add merge operand to RISCVISD::VRGATHER*_VL nodes.
ClosedPublic

Authored by craig.topper on Jun 16 2022, 7:38 PM.

Details

Summary

Use it in place of VSELECT_VL+VRGATHER*_VL.

This simplifies the isel patterns.

Overall, I think trying to match select+op to create masked instructions
in isel doesn't scale. We either need to do it in DAG combine, pre-isel
peepole, or post-isel peephole. I don't yet know which is the right
answer, but for this case it seemed best to be able to request the
masked form directly from lowering.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 16 2022, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:38 PM
craig.topper requested review of this revision.Jun 16 2022, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:38 PM

Seems reasonable to me, it is hard to see how select+op patterns could ever scale on large ISA like this. I've done a post-isel MIR peephole for this sort of thing in the past. We could perhaps also do a DAG post-process like we do for masked/unmasked RVV.

On the patch: we probably need to update the ISD opcode comment in RISCVISelLowering.h? We do comment passthru/merge operands though I don't know if our coverage is 100%.

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
1841–1842

I'm a bit out of the loop, but could the DAG post-process turn masked vrgathers into unmasked ones? Could we remove the true_mask patterns while we're at it?

Update description in RISCVISelLowering.h

Looks reasonable to me, though the TD changes are a bit more involved than I'm comfortable signing off on just yet. Will defer to Fraser.

craig.topper added inline comments.Jun 17 2022, 12:35 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
1841–1842

Fraser knows this but I'll mention it for @reames. srcvalue happens to be hole in the isel pattern system that causes the operand to be completely ignored. Since the mask is true_mask here we don't need the value. This allows the operand to be dropped even if it isn't undef. I think we get an error if we capture an operand and then don't use it in the output pattern.

This revision is now accepted and ready to land.Jun 20 2022, 8:42 AM
This revision was landed with ongoing or failed builds.Jun 20 2022, 7:16 PM
This revision was automatically updated to reflect the committed changes.