Skip to content

[RISCV] Unaligned vec mem => prefer alt opc vec #154153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[RISCV] Unaligned vec mem => prefer alt opc vec #154153

wants to merge 2 commits into from
+400 −117

Conversation

mgudim
Copy link
Contributor

@mgudim mgudim commented Aug 18, 2025 •
edited
Loading

Return true in RISCVTTIImpl::preferAlternateOpcodeVectorization if
subtarget supports unaligned memory accesses.

@mgudim mgudim requested a review from alexey-bataev August 18, 2025 16:31
@llvmbot
Copy link
Member

llvmbot commented Aug 18, 2025 •
edited
Loading

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-risc-v

Author: Mikhail Gudim (mgudim)

Changes

are supported.

Full diff: https://github.com/llvm/llvm-project/pull/154153.diff

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp (+4)
  • (modified) llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h (+1-1)
diff --git a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
index 85b3059d87da7..f49c3ae9bdea3 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
@@ -2713,6 +2713,10 @@ unsigned RISCVTTIImpl::getMinTripCountTailFoldingThreshold() const {
   return RVVMinTripCount;
 }
 
+bool RISCVTTIImpl::preferAlternateOpcodeVectorization() const override {
+  return ST->enableUnalignedVectorMem();
+}
+
 TTI::AddressingModeKind
 RISCVTTIImpl::getPreferredAddressingMode(const Loop *L,
                                          ScalarEvolution *SE) const {
diff --git a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
index 6a1f4b3e3bedf..254908f97186c 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
+++ b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
@@ -132,7 +132,7 @@ class RISCVTTIImpl final : public BasicTTIImplBase<RISCVTTIImpl> {
 
   unsigned getMaximumVF(unsigned ElemWidth, unsigned Opcode) const override;
 
-  bool preferAlternateOpcodeVectorization() const override { return false; }
+  bool preferAlternateOpcodeVectorization() const override;
 
   bool preferEpilogueVectorization() const override {
     // Epilogue vectorization is usually unprofitable - tail folding or

@mgudim mgudim mentioned this pull request Aug 18, 2025
Open
alexey-bataev
alexey-bataev reviewed Aug 18, 2025
View reviewed changes
Copy link
Member

@alexey-bataev alexey-bataev left a comment

Choose a reason for hiding this comment

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

Tests

@mgudim mgudim force-pushed the prefer_alt_opc_vec branch from ae909f8 to 0f790de Compare August 18, 2025 16:49
@topperc
Copy link
Collaborator

topperc commented Aug 18, 2025

vectorization is misspelled in title. Title is split into the description.

@mgudim mgudim force-pushed the prefer_alt_opc_vec branch from 0f790de to fdb931f Compare August 18, 2025 20:00
@mgudim
Copy link
Contributor Author

mgudim commented Aug 18, 2025

Tests

@alexey-bataev I am actually struggling to come up with a simple test where split vectorization would trigger. Can you please point me to an example.

@mgudim mgudim changed the title [RISCV] Prefer alt opcode vectorirazion if unaligned vector mem accesses [RISCV] Unaligned vec mem => prefer alt opc vec Aug 18, 2025
wangpc-pp
wangpc-pp reviewed Aug 19, 2025
View reviewed changes
llvm/test/Transforms/SLPVectorizer/RISCV/alt-opc-vectorization.ll Outdated
; UNALIGNED_VEC_MEM-NEXT: store <8 x i8> [[TMP2]], ptr [[GEP_S0]], align 1
; UNALIGNED_VEC_MEM-NEXT: ret void
;
; NO_UNALIGNED_VEC_MEM-LABEL: define void @alternate_opcodes(
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference? I may miss it…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No difference. I am trying to trigger the split vectorization path, but I can' t yet come up with simple IR to do that

@mgudim
Copy link
Contributor Author

mgudim commented Aug 23, 2025

Tests

@alexey-bataev I am actually struggling to come up with a simple test where split vectorization would trigger. Can you please point me to an example.

ping

@mgudim mgudim force-pushed the prefer_alt_opc_vec branch from fdb931f to 2f26eb4 Compare August 28, 2025 15:14
alexey-bataev
alexey-bataev reviewed Aug 28, 2025
View reviewed changes
llvm/test/Transforms/SLPVectorizer/RISCV/complex-loads.ll Outdated
@@ -1,6 +1,7 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
; RUN: opt -S -mtriple riscv64-unknown-linux-gnu < %s --passes=slp-vectorizer -mattr=+v | FileCheck %s
; RUN: opt -S -mtriple riscv64-unknown-linux-gnu < %s --passes=slp-vectorizer -mattr=+v -slp-threshold=-15 | FileCheck %s --check-prefix=THR15
; RUN: opt -S -mtriple riscv64-unknown-linux-gnu < %s --passes=slp-vectorizer -mattr=+v,+unaligned-vector-mem | FileCheck %s --check-prefix=UNALIGNED_VEC_MEM
Copy link
Member

Choose a reason for hiding this comment

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

Does it affect TTI? Particularly, does it affect isLegalMaskedLoadStore? I do not see the difference in the results.
Need to teach TTI, that it is legal to have alignment less than the basic load type. I think you can try to do it for X86, I think it supports it.

Copy link
Contributor Author

@mgudim mgudim Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Need to teach TTI, that it is legal to have alignment less than the basic load type

I think this is already present in isLegalMaskedLoadStore:

if (!ST->enableUnalignedVectorMem() && Alignment < ElemType.getStoreSize())
   return false;

isLegalMaskedLoadStore returns true.

Copy link
Member

Choose a reason for hiding this comment

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

Could you check, please, if the test results are actually updated? Also, you can try to run test/Transforms/PhaseOrdering/AArch64/slpordering.ll, but try to run it for RISC-V and see, if it changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry. It seems like I updated the test without building opt first.

Copy link
Member

Choose a reason for hiding this comment

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

Could you split this change and pre-commit the test at first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#156110

Mikhail Gudim added 2 commits August 29, 2025 15:19
[RISCV] Unaligned vec mem => prefer alt opc vec
569f60b 
Return `true` in `RISCVTTIImpl::preferAlternateOpcodeVectorization` if
subtarget supports unaligned memory accesses.
updated checks in complex-loads.ll correctly.
efdd5b5
@mgudim mgudim force-pushed the prefer_alt_opc_vec branch from cdcb2f5 to efdd5b5 Compare August 29, 2025 22:23
mgudim
mgudim commented Aug 29, 2025
View reviewed changes
llvm/test/Transforms/SLPVectorizer/RISCV/complex-loads.ll
@@ -275,135 +275,414 @@ define i32 @test(ptr %pix1, ptr %pix2, i64 %idx.ext, i64 %idx.ext63, ptr %add.pt
; UNALIGNED_VEC_MEM-LABEL: define i32 @test(
; UNALIGNED_VEC_MEM-SAME: ptr [[PIX1:%.*]], ptr [[PIX2:%.*]], i64 [[IDX_EXT:%.*]], i64 [[IDX_EXT63:%.*]], ptr [[ADD_PTR:%.*]], ptr [[ADD_PTR64:%.*]]) #[[ATTR0:[0-9]+]] {
; UNALIGNED_VEC_MEM-NEXT: entry:
; UNALIGNED_VEC_MEM-NEXT: [[TMP54:%.*]] = load i8, ptr [[PIX1]], align 1
Copy link
Contributor Author

@mgudim mgudim Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So it seems like nothing gets vectorized now, is that what we should expect?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

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

@wangpc-pp wangpc-pp wangpc-pp left review comments

@alexey-bataev alexey-bataev alexey-bataev approved these changes

Assignees
No one assigned
Labels
backend:RISC-V llvm:transforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mgudim @llvmbot @topperc @alexey-bataev @wangpc-pp