Skip to content

Conversation

@NickGuy-Arm
Copy link
Contributor

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Jan 20, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Nicholas Guy (NickGuy-Arm)

Changes

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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/VPlan.h (+2-1)
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 87f87bf1437196..9d8136a0ebf57d 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -2459,7 +2459,8 @@ class VPPartialReductionRecipe : public VPSingleDefRecipe {
   ~VPPartialReductionRecipe() override = default;
 
   VPPartialReductionRecipe *clone() override {
-    return new VPPartialReductionRecipe(Opcode, getOperand(0), getOperand(1));
+    return new VPPartialReductionRecipe(Opcode, getOperand(0), getOperand(1),
+       getUnderlyingInstr());
   }
 
   VP_CLASSOF_IMPL(VPDef::VPPartialReductionSC)

@github-actions
Copy link

github-actions bot commented Jan 20, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@fhahn
Copy link
Contributor

fhahn commented Jan 20, 2025

IIUC this should only change VPlan printing or is the underlying instr used by other parts of the recipe? If it's the former, I guess to could be tested by check the printed VPlan during epilogue vectorization?

@NickGuy-Arm NickGuy-Arm force-pushed the partial-reduce-recipe-fixup branch from cf56562 to 621cd7b Compare January 21, 2025 11:41
; CHECK-EMPTY:
; CHECK-NEXT: <x1> vector loop: {
; CHECK-NEXT: vector.body:
; CHECK-NEXT: SCALAR-PHI vp<%0> = phi ir<0>, vp<%index.next>
Copy link
Contributor

Choose a reason for hiding this comment

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

please use patterns to match the vp<> value numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

; REQUIRES: asserts
; RUN: opt -mattr=+neon,+dotprod -passes=loop-vectorize -debug-only=loop-vectorize -force-vector-interleave=1 -enable-epilogue-vectorization -epilogue-vectorization-force-VF=2 -disable-output %s 2>&1 | FileCheck %s

target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the test. Do we need a separate test file or could we just enable epilogue vectorization in the existing llvm/test/Transforms/LoopVectorize/AArch64/vplan-printing.ll?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I was a bit skeptical at first about merging them, but they went together with no issues.

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@NickGuy-Arm NickGuy-Arm merged commit 26b61e1 into llvm:main Jan 23, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants