Page MenuHomePhabricator

[SelectionDAG] Add stub implementation of ReplaceInsertSubVectorResults
AbandonedPublic

Authored by joechrisellis on May 19 2021, 5:59 AM.

Details

Summary

This is required to prevent hitting an llvm_unreachable when
ReplaceNodeResults is called on an INSERT_SUBVECTOR node. At the moment,
this implementation simply delegates to common code. It is likely that
in the future we will want to change this, but for now, the stub
implementation is sufficient to prevent crashing.

This patch is part of a wider piece of work to allow INSERT_SUBVECTOR to
be lowered when inserting into a smaller-than-legal scalable type. Any
code examples that hit the bad codepath will hit other crashes further
down the line, so this patch does not include tests. Tests which guard
against the crashing behaviour will be added in a later patch.

Diff Detail

Event Timeline

joechrisellis created this revision.May 19 2021, 5:59 AM
joechrisellis requested review of this revision.May 19 2021, 5:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 5:59 AM
peterwaller-arm accepted this revision.May 19 2021, 8:21 AM
This revision is now accepted and ready to land.May 19 2021, 8:21 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16728–16730

I'm not saying this is not the correct solution but you typically opt into having custom handling via ReplaceNodeResults so perhaps we've requested (via setOperationAction()) something we didn't want?

joechrisellis added inline comments.May 21 2021, 5:34 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16728–16730

That's a good point. I reckon we're hitting the unreachable because of the following code block in AArch64ISelLowering.cpp:

// Illegal unpacked integer vector types.
for (auto VT : {MVT::nxv8i8, MVT::nxv4i16, MVT::nxv2i32}) {
  setOperationAction(ISD::EXTRACT_SUBVECTOR, VT, Custom);
  setOperationAction(ISD::INSERT_SUBVECTOR, VT, Custom); // <--
}

I haven't done much digging, but I think that's important. Reverting this patch locally, then removing that line fixes the issues that this patch resolves, but means that the following tests fail:

llvm/test/CodeGen/AArch64/sve-masked-gather-legalize.ll
llvm/test/CodeGen/AArch64/sve-split-fcvt.ll
llvm/test/CodeGen/AArch64/sve-split-trunc.ll

with Do not know how to promote this operator's operand!. This makes me think we probably need to keep those lines.

paulwalker-arm added inline comments.May 21 2021, 6:31 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16728–16730

That makes sense, thanks for checking @joechrisellis .

In which I'll just say there doesn't seem much value in creating an empty function, but rather can just add a comment here along the lines of "We've requested custom lowering but rely on common code for result type legalisation." and return directly.

Given this'll make this a two line change without any tests I think you may as well just add the code to D102766.

@paulwalker-arm ACK -- will fold this into D102766. 🙂

joechrisellis abandoned this revision.May 21 2021, 8:12 AM

This change will be folded into D102766.