[DAG] Teach SelectionDAGBuilder to read parameter alignment of compressstore/expandload. #83763

yetingk · 2024-03-04T04:58:02Z

Previously SelectionDAGBuilder used ABI alignment for compressstore/expandload. This patch allows SelectionDAGBuilder to use parameter alignment like vp intrinsics. This does not follow the original code to default use vector type alignment, since it is possible implemented to unaligned vector alignment.

llvmbot · 2024-03-04T04:58:32Z

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: Yeting Kuo (yetingk)

Changes

Previously SelectionDAGBuilder used ABI alignment for compressstore/expandload. This patch allows SelectionDAGBuilder to use parameter alignment like vp intrinsics and stills uses ABI alignment for them when they don't have alignment attriubtes.

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

3 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+4)
(modified) llvm/test/CodeGen/X86/masked_compressstore_isel.ll (+17-1)
(added) llvm/test/CodeGen/X86/masked_expandload_isel.ll (+33)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index ab2f42d2024ccc..39628f4fb3689d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4582,6 +4582,8 @@ void SelectionDAGBuilder::visitMaskedStore(const CallInst &I,
     Ptr = I.getArgOperand(1);
     Mask = I.getArgOperand(2);
     Alignment = std::nullopt;
+    if (MaybeAlign Align = I.getParamAlign(1))
+      Alignment = Align;
   };
 
   Value  *PtrOperand, *MaskOperand, *Src0Operand;
@@ -4746,6 +4748,8 @@ void SelectionDAGBuilder::visitMaskedLoad(const CallInst &I, bool IsExpanding) {
     // @llvm.masked.expandload.*(Ptr, Mask, Src0)
     Ptr = I.getArgOperand(0);
     Alignment = std::nullopt;
+    if (MaybeAlign Align = I.getParamAlign(0))
+      Alignment = Align;
     Mask = I.getArgOperand(1);
     Src0 = I.getArgOperand(2);
   };
diff --git a/llvm/test/CodeGen/X86/masked_compressstore_isel.ll b/llvm/test/CodeGen/X86/masked_compressstore_isel.ll
index 1851a21c8c0641..b5857b22382da0 100644
--- a/llvm/test/CodeGen/X86/masked_compressstore_isel.ll
+++ b/llvm/test/CodeGen/X86/masked_compressstore_isel.ll
@@ -7,7 +7,7 @@ entry:
   ret void
 }
 
-; CHECK-LABEL: bb.0.entry:
+; CHECK-LABEL:   name: _Z3fooiPiPs
 ; CHECK:         %1:vr128x = COPY $xmm1
 ; CHECK-NEXT:    %0:vr256x = COPY $ymm0
 ; CHECK-NEXT:    %2:vr128x = VPSLLWZ128ri %1, 15
@@ -16,6 +16,22 @@ entry:
 ; CHECK-NEXT:    VPCOMPRESSWZ128mrk $noreg, 1, $noreg, 0, $noreg, killed %3, killed %4 :: (store unknown-size into `ptr null`, align 16)
 ; CHECK-NEXT:    RET 0
 
+define void @_Z3foo2iPiPs(<8 x i32> %gepload, <8 x i1> %0) #0 {
+entry:
+  %1 = trunc <8 x i32> %gepload to <8 x i16>
+  tail call void @llvm.masked.compressstore.v8i16(<8 x i16> %1, ptr align 32 null, <8 x i1> %0)
+  ret void
+}
+
+; CHECK-LABEL:   name: _Z3foo2iPiPs
+; CHECK:         %1:vr128x = COPY $xmm1
+; CHECK-NEXT:    %0:vr256x = COPY $ymm0
+; CHECK-NEXT:    %2:vr128x = VPSLLWZ128ri %1, 15
+; CHECK-NEXT:    %3:vk16wm = VPMOVW2MZ128rr killed %2
+; CHECK-NEXT:    %4:vr128x = VPMOVDWZ256rr %0
+; CHECK-NEXT:    VPCOMPRESSWZ128mrk $noreg, 1, $noreg, 0, $noreg, killed %3, killed %4 :: (store unknown-size into `ptr null`, align 32)
+; CHECK-NEXT:    RET 0
+
 ; Function Attrs: nocallback nofree nosync nounwind willreturn memory(argmem: write)
 declare void @llvm.masked.compressstore.v8i16(<8 x i16>, ptr nocapture, <8 x i1>) #1
 
diff --git a/llvm/test/CodeGen/X86/masked_expandload_isel.ll b/llvm/test/CodeGen/X86/masked_expandload_isel.ll
new file mode 100644
index 00000000000000..7f20e03d343cdb
--- /dev/null
+++ b/llvm/test/CodeGen/X86/masked_expandload_isel.ll
@@ -0,0 +1,33 @@
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -start-after=codegenprepare -stop-before finalize-isel | FileCheck %s
+
+define <8 x i16> @_Z3fooiPiPs(<8 x i16> %src, <8 x i1> %0) #0 {
+entry:
+  %res = call <8 x i16> @llvm.masked.expandload.v8i16(ptr null, <8 x i1> %0, <8 x i16> %src)
+  ret <8 x i16> %res
+}
+
+; CHECK-LABEL:   name: _Z3fooiPiPs
+; CHECK:         %1:vr128x = COPY $xmm1
+; CHECK-NEXT:    %0:vr128x = COPY $xmm0
+; CHECK-NEXT:    %2:vr128x = VPSLLWZ128ri %1, 15
+; CHECK-NEXT:    %3:vk16wm = VPMOVW2MZ128rr killed %2
+; CHECK-NEXT:    %4:vr128x = VPEXPANDWZ128rmk %0, killed %3, $noreg, 1, $noreg, 0, $noreg :: (load unknown-size from `ptr null`, align 16)
+
+define <8 x i16> @_Z3foo2iPiPs(<8 x i16> %src, <8 x i1> %0) #0 {
+entry:
+  %res = call <8 x i16> @llvm.masked.expandload.v8i16(ptr align 32 null, <8 x i1> %0, <8 x i16> %src)
+  ret <8 x i16> %res
+}
+
+; CHECK-LABEL:   name: _Z3foo2iPiPs
+; CHECK:         %1:vr128x = COPY $xmm1
+; CHECK-NEXT:    %0:vr128x = COPY $xmm0
+; CHECK-NEXT:    %2:vr128x = VPSLLWZ128ri %1, 15
+; CHECK-NEXT:    %3:vk16wm = VPMOVW2MZ128rr killed %2
+; CHECK-NEXT:    %4:vr128x = VPEXPANDWZ128rmk %0, killed %3, $noreg, 1, $noreg, 0, $noreg :: (load unknown-size from `ptr null`, align 32)
+
+; Function Attrs: nocallback nofree nosync nounwind willreturn memory(argmem: write)
+declare <8 x i16> @llvm.masked.expandload.v8i16(ptr, <8 x i1>, <8 x i16>)
+
+attributes #0 = { "target-cpu"="icelake-server" }
+attributes #1 = { nocallback nofree nosync nounwind willreturn memory(argmem: write) }

arsenm · 2024-03-04T05:18:20Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

+    if (MaybeAlign Align = I.getParamAlign(0))
+      Alignment = Align;


can just assign Alignment from I.getParamAlign?

I think we should be using I.getParamAlign(0).getValueOr(1). The call to DAG.getEVTAlign(VT) later doesn't make sense for a compressed store or expanding load. The vector type isn't really relevant. Maybe we would use the alignment for the element type, but 1 is conservatively correct.

To clarify the reason I think the vector type isn't relevant, is that we are like not accessing a full vector of memory. If this intrinsic is used in a loop, the pointer for the next iteration will likely have advanced less than a full vector making it not aligned.

I had addressed Craig's comment.

MaybeAlign shouldn't exist. We should just make all of the contexts behave like getValueOr1

I had already replaced MaybeAlign by Align in 6322e3d

arsenm · 2024-03-04T08:15:28Z

llvm/test/CodeGen/X86/masked_expandload_isel.ll

@@ -0,0 +1,33 @@
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -start-after=codegenprepare -stop-before finalize-isel | FileCheck %s


why do you need -start-after?

This file is almost copied from originally existed file llvm/test/CodeGen/X86/masked_compressstore_isel.ll I think the reason is to avoid ScalarizeMaskedMemIntrin pass scalarizing the intrinsic.

Wouldn't that not happen if it's legal for the target?

Yes. it doesn't happened for this case. I just guessed the reason about that.

Use update_mir_test_checks.py. I'll fix the compress store test to use the script and we can rebase this on top of that

Please rebase after bdfebc3

arsenm · 2024-03-04T08:15:34Z

llvm/test/CodeGen/X86/masked_expandload_isel.ll

@@ -0,0 +1,33 @@
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -start-after=codegenprepare -stop-before finalize-isel | FileCheck %s
+
+define <8 x i16> @_Z3fooiPiPs(<8 x i16> %src, <8 x i1> %0) #0 {


Use named values in tests

Should I also update naming in llvm/test/CodeGen/X86/masked_compressstore_isel.ll?

I had already updated llvm/test/CodeGen/X86/masked_expandload_isel.ll.

efriedma-quic · 2024-03-04T08:54:37Z

It's probably worth documenting the behavior in LangRef, like we do for other intrinsics that support "align" markings.

…ndload sections. align attribute is used for masked.compress/expandload in commit llvm#83519, llvm#83763, llvm#83516.

topperc · 2024-03-05T00:00:08Z

llvm/test/CodeGen/X86/masked_compressstore_isel.ll

+; CHECK-NEXT:    %2:vr128x = VPSLLWZ128ri %1, 15
+; CHECK-NEXT:    %3:vk16wm = VPMOVW2MZ128rr killed %2
+; CHECK-NEXT:    %4:vr128x = VPMOVDWZ256rr %0
+; CHECK-NEXT:    VPCOMPRESSWZ128mrk $noreg, 1, $noreg, 0, $noreg, killed %3, killed %4 :: (store unknown-size into `ptr null`, align 1)


Should this have align 16 instead of align 1?

oh nevermind the check lines are just in a funny spot.

…ssstore/expandload. Previously SelectionDAGBuilder used ABI alignment for compressstore/expandload. This patch allows SelectionDAGBuilder to use parameter alignment like memory vp intrinsics and stills uses ABI alignment for them when they don't have alignment attriubtes.

topperc

LGTM

topperc · 2024-03-05T04:20:08Z

what happened to the LangRef change?

topperc · 2024-03-05T04:21:25Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

    // llvm.masked.store.*(Src0, Ptr, alignment, Mask)
    Src0 = I.getArgOperand(0);
    Ptr = I.getArgOperand(1);
-    Alignment = cast<ConstantInt>(I.getArgOperand(2))->getMaybeAlignValue();
+    Alignment = cast<ConstantInt>(I.getArgOperand(2))->getAlignValue();


were any tests passing 0?

The LangRef tells that the alignment value of masked.load/store should be power of 2, so I think we could ignore the case.

yetingk · 2024-03-05T06:28:40Z

what happened to the LangRef change?

Sorry, do you mean why I changed LangRef? I changed it because @efriedma-quic asked for. And I opened another PR for this just I think changing ir document is a little exceed the scope of this patch.

It's probably worth documenting the behavior in LangRef, like we do for other intrinsics that support "align" markings.

…ndload sections. (#83808) Align attribute has already been used for masked.compress/expandload in commit #83519, #83763 and #83516.

yetingk requested review from preames, topperc and xiangzh1 March 4, 2024 04:58

llvmbot added backend:X86 llvm:SelectionDAG SelectionDAGISel as well labels Mar 4, 2024

arsenm reviewed Mar 4, 2024

View reviewed changes

llvmbot added the llvm:ir label Mar 4, 2024

yetingk force-pushed the dagbuilder branch from c3b7ee3 to 9d3a50b Compare March 4, 2024 09:04

yetingk removed the llvm:ir label Mar 4, 2024

yetingk mentioned this pull request Mar 4, 2024

[LangRef] Add aligned attribute description into masked.compress/expandload sections. #83808

Merged

topperc reviewed Mar 5, 2024

View reviewed changes

yetingk force-pushed the dagbuilder branch from 9d3a50b to a5c9304 Compare March 5, 2024 01:58

topperc approved these changes Mar 5, 2024

View reviewed changes

Use Align instead of MaybeAlign.

6322e3d

topperc reviewed Mar 5, 2024

View reviewed changes

yetingk added a commit that referenced this pull request Mar 5, 2024

[LangRef] Add aligned attribute description into masked.compress/expa…

6862233

…ndload sections. (#83808) Align attribute has already been used for masked.compress/expandload in commit #83519, #83763 and #83516.

arsenm approved these changes Mar 5, 2024

View reviewed changes

yetingk merged commit d95a0d7 into llvm:main Mar 5, 2024

		if (MaybeAlign Align = I.getParamAlign(0))
		Alignment = Align;

		@@ -0,0 +1,33 @@
		; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -start-after=codegenprepare -stop-before finalize-isel \| FileCheck %s

		@@ -0,0 +1,33 @@
		; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -start-after=codegenprepare -stop-before finalize-isel \| FileCheck %s

		define <8 x i16> @_Z3fooiPiPs(<8 x i16> %src, <8 x i1> %0) #0 {

[DAG] Teach SelectionDAGBuilder to read parameter alignment of compressstore/expandload. #83763

[DAG] Teach SelectionDAGBuilder to read parameter alignment of compressstore/expandload. #83763

Uh oh!

Conversation

yetingk commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yetingk Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yetingk Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efriedma-quic commented Mar 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

topperc commented Mar 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yetingk commented Mar 5, 2024

Uh oh!

Uh oh!

yetingk commented Mar 4, 2024 •

edited

Loading

llvmbot commented Mar 4, 2024 •

edited

Loading

topperc Mar 4, 2024 •

edited

Loading

yetingk Mar 4, 2024 •

edited

Loading

yetingk Mar 4, 2024 •

edited

Loading