This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Add support for reductions in ComplexDeinterleaving pass
ClosedPublic

Authored by igor.kirillov on Jun 2 2023, 12:18 PM.

Details

Summary

This commit enhances the ComplexDeinterleaving pass to handle unordered
reductions in simple one-block vectorized loops, supporting both
SVE and Neon architectures.

Diff Detail

Event Timeline

igor.kirillov created this revision.Jun 2 2023, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2023, 12:18 PM
igor.kirillov requested review of this revision.Jun 2 2023, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2023, 12:18 PM
igor.kirillov edited the summary of this revision. (Show Details)

Add tests

Matt added a subscriber: Matt.Jun 2 2023, 3:35 PM

Looks good, just a couple of small points from me today.

llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp
393–401

Might be worth adding a comment here saying what it returns, it's not immediately clear to me

1745–1798

This function is getting big. Solely in terms of readability, I'd suggest moving the code for each condition to separate functions, leaving the ifs here (at least the ReductionOperation branch, ReductionPHI is small enough on its own)

Refactor replaceNode. Add comments

igor.kirillov marked an inline comment as done.Jun 7 2023, 9:50 AM
igor.kirillov added inline comments.
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp
1745–1798

What do you think about this version?

Made changes to the checkNodes method in how it verifies reduction nodes.
Additionally, the final reduction operation is now generated at the beginning of the basic block to prevent broken IR.

NickGuy accepted this revision.Jun 12 2023, 4:18 AM

LGTM, nice work

llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp
1797

Nit: May be worth adding a default case for this switch. Something simple like the following

default:
  llvm_unreachable("Unhandled case in ComplexDeinterleavingGraph::replaceNode");
  break;
This revision is now accepted and ready to land.Jun 12 2023, 4:18 AM
igor.kirillov edited the summary of this revision. (Show Details)

Rebase before pushing

Add default unreachable case

This revision was landed with ongoing or failed builds.Jun 14 2023, 10:28 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Sep 6 2023, 8:02 AM
fhahn added inline comments.
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp
1445

Iterating over std::map<Instruction *, ... here causes codegen non-determinism as the order depends on the memory addresses, so the order may different across different runs.

I pushed a fix to use MapVector instead: https://github.com/llvm/llvm-project/commit/4c9223c77062bc93f63039c9ebdd885d0f50de59

Spotting such issues can be very difficult and we need to be careful when the iteration order can be non-deterministic