Skip to content

[AIE] Fix instruction-select crash on unpack of a non-load value#1025

Open
hunhoffe wants to merge 3 commits into
aie-publicfrom
fix-unpack-non-load-crash
Open

[AIE] Fix instruction-select crash on unpack of a non-load value#1025
hunhoffe wants to merge 3 commits into
aie-publicfrom
fix-unpack-non-load-crash

Conversation

@hunhoffe

Copy link
Copy Markdown
Collaborator

Note: I'm just a peano user, not a dev -- agents helped copiously in this PR, so I'd appreciate a thorough review.

Problem

Feeding the result of acc.to_vector<int8>() directly into aie::unpack crashes Peano with a SIGSEGV during instruction selection, on both AIE2 and AIE2P.

Minimal C++ reproducer (clang++ --target=aie2p-none-unknown-elf -O2):

  #include <aie_api/aie.hpp>
  aie::vector<int16, 32> bad(aie::vector<int8, 32> a, aie::vector<int8, 64> b) {
    using MMUL = aie::mmul<4, 8, 8, int8, int8>;
    MMUL acc; acc.mac(a, b);
    aie::vector<int8, 32> proj = acc.to_vector<int8>(8);
    return aie::unpack(proj);            // <- proj feeds unpack directly
  } 

Backtrace:

  • AIE2PInstructionSelector::getCombinedOpcodeUNPACKLoad
  • AIE2PInstructionSelector::canCombineUNPACKLoad
  • AIEBaseInstructionSelector::selectG_AIE_LOAD_UNPACK
  • llvm::InstructionSelect::selectMachineFunction

Root cause

selectG_AIE_LOAD_UNPACK tries to fold a load directly into the unpack. It gates the fold on MemOp.mayLoad(), then calls getLoadStoreSize(MemOp), which dereferences the instruction's first memory operand:

  return (*MI.memoperands_begin())->getSizeInBits().getValue();

The producer here is the SRS narrowing intrinsic (the lowering of to_vector<int8>()), a G_INTRINSIC_W_SIDE_EFFECTS that reports mayLoad() but carries no memory operand. memoperands_begin() == memoperands_end(), so the dereference reads an invalid pointer → segfault.

Fix

Bail out of the combine when the candidate has no memory operand, so a non-load producer falls back to normal VUNPACK selection. Applied to both the AIE2 and AIE2P selectors (both had the identical latent crash).

Testing

  • New MIR regression tests for both targets (inst-select-unpack-non-load.mir); they SIGSEGV before this change and select cleanly after.
  • Real load→unpack still folds into VLDB_UNPACK (existing inst-select-*vldb_unpack.mir tests unchanged).
  • Full llvm/test/CodeGen/AIE/{aie2,aie2p}/GlobalIS*el suites pass locally (391/391).

selectG_AIE_LOAD_UNPACK assumes the def feeding an unpack intrinsic is a
load, but it only checks mayLoad() before calling getLoadStoreSize(), which
dereferences the instruction's first memory operand. The SRS narrowing
intrinsic (the lowering of acc.to_vector<int8>()) reports mayLoad() yet
carries no memory operand, so feeding its result directly into aie::unpack
crashed instruction-select with a SIGSEGV.

Guard the combine on memoperands_empty() in both the AIE2 and AIE2P
selectors so a non-load producer falls back to normal VUNPACK selection.

Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
// memory access (e.g. the SRS narrowing intrinsic that lowers
// acc.to_vector<int8>()). Such ops carry no memory operand, so bail out
// before getLoadStoreSize() dereferences a non-existent one.
if (!MemOp.mayLoad() || MemOp.memoperands_empty())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried AIE2PS? If it doesn't occur there, it may mean that the memory operand modeling/handling is better there.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martien-de-jong Verified — AIE2P crashes identically before this fix. The guard logic in AIE2PInstructionSelector::getCombinedOpcodeUNPACKLoad was if (!MemOp.mayLoad()) return {}; with no memoperands_empty() check, so the same null-deref SIGSEGV occurs. I confirmed this by running the new inst-select-unpack-non-load.mir test against an unpatched build (exit 139 / SIGSEGV) and a patched build (exit 0, selects VUNPACK_mv_unpack_w_unpackSign1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants