[X86][FastIsel] Teach how to select vector load instructions.

[andreadb]

Summary

This patch teaches fast-isel how to select 128-bit vector load instructions.

I simply extended the switch statement in method 'X86FastEmitLoad' adding extra cases for 128-bit vector types.
I had to slightly modify the signature of 'X86FastEmitLoad' to allow selecting opcodes based on alignment information.

Added test CodeGen/X86/fast-isel-vecload.ll

Please let me know if ok to submit.

Thanks,
Andrea

[andreadb]

Uploaded a new version of the patch.
This time with a correct test file that checks both SSE and AVX vector loads.

-Andrea

[qcolombet]

lib/Target/X86/X86FastISel.cpp

1099

By default, don’t we assume stuff are unaligned, i.e., Alignment = 1?

[andreadb]

Hi Quentin,

lib/Target/X86/X86FastISel.cpp

1099

That's what I thought originally. However, when testing this patch, I noticed that without fast-isel I was always getting aligned load instructions.

Example:

define <4 x float> @foo(<4 x float>* %V) {
entry:
  %0 = load <4 x float>, <4 x float>* %V
  ret <4 x float> %0
}

without -fast-isel, llc -mattr=+sse2 generates a vmovaps.

As far as I understand, if a load doesn't specify the alignment, then the backend assumes the 'ABIAlignment' based on the value type.

This also matches the behavior of method 'X86FastISel::X86SelectStore'. At around line 934, we check for the alignment of the store in input; if a store node doesn't specify any alignment (i.e. Alignment is 0), then FastISel implicitly sets Alignment to the 'ABIAlignment'.
For the record: the code between lines 1096 and 1099 is mostly copy-pasted from that method.

In general I don't have a strong opinion on this: I can change line 1099 so that we propagate value 1 to Alignment. However, if we do this, then for consistency, we should probably also change how we check the alignment of store instructions in 'X86SelectStore'. What do you think?

[qcolombet]

lib/Target/X86/X86FastISel.cpp

331

Instead of passing a bool, I think the actual alignment we saw may be useful.
We could imagine that we generate different instructions based on the value of that alignment.

That being said, this is not the case right now, so we can adjust accordingly later. It just feel weird that the callers know about the required alignments.

What do you think?

1099

It seems we do the same for SelectionDAG, at least at some places, unlike what I thought.
May be worth bringing that up on the dev mailing list.

Anyhow, this part looks good as it is consistent with what exists.

[hfinkel]

lib/Target/X86/X86FastISel.cpp

1099

The general convention all throughout LLVM is that Alignment == 0 means to default to the required ABI alignment. You need Alignment == 1 to get completely unaligned.

[andreadb]

lib/Target/X86/X86FastISel.cpp

331

Right, I agree with you; it feels wrong that the caller sets the 'Aligned' flag without knowing which opcode will be selected for the load instruction.
I am going to replace 'bool Aligned' we 'unsigned Alignment'. I will then change the logic inside this method so that an 'aligned' load is selected only if Alignment is != 16.

Thanks!

[andreadb]

Patch updated.
Method 'X86FastEmitLoad' now takes as input an unsigned 'Alignment' value (instead of a 'bool Aligned'). We now select aligned vector loads only if Alignment is >= 16.
Added extra test cases in CodeGen/X86/fast-isel-vecload.ll to check that we correctly select "ABI aligned" vector loads.

Please let me know what you think.

Thanks again for your time!
-Andrea

[qcolombet]

Thanks Andrea!

LGTM.

@Hal, thanks for the clarifications.

Q.

[andreadb]

Thanks Quentin.
Committed revision 233270.

lib/Target/X86/X86FastISel.cpp

public:
   bool fastLowerIntrinsicCall(const IntrinsicInst *II) override;
 
 #include "X86GenFastISel.inc"
 
 private:
   bool X86FastEmitCompare(const Value *LHS, const Value *RHS, EVT VT, DebugLoc DL);
 
   bool X86FastEmitLoad(EVT VT, const X86AddressMode &AM, MachineMemOperand *MMO,
-                       unsigned &ResultReg);
+                       unsigned &ResultReg, bool Aligned = false);
 
   bool X86FastEmitStore(EVT VT, const Value *Val, const X86AddressMode &AM,
                         MachineMemOperand *MMO = nullptr, bool Aligned = false);
   bool X86FastEmitStore(EVT VT, unsigned ValReg, bool ValIsKill,
                         const X86AddressMode &AM,
                         MachineMemOperand *MMO = nullptr, bool Aligned = false);
 
   bool X86FastEmitExtend(ISD::NodeType Opc, EVT DstVT, unsigned Src, EVT SrcVT,

 }
 
 #include "X86GenCallingConv.inc"
 
 /// X86FastEmitLoad - Emit a machine instruction to load a value of type VT.
 /// The address is either pre-computed, i.e. Ptr, or a GlobalAddress, i.e. GV.
 /// Return true and the result register by reference if it is possible.
 bool X86FastISel::X86FastEmitLoad(EVT VT, const X86AddressMode &AM,
-                                  MachineMemOperand *MMO, unsigned &ResultReg) {
+                                  MachineMemOperand *MMO, unsigned &ResultReg,
+                                  bool Aligned) {
   // Get opcode and regclass of the output for the given load instruction.
   unsigned Opc = 0;
   const TargetRegisterClass *RC = nullptr;
   switch (VT.getSimpleVT().SimpleTy) {
   default: return false;
   case MVT::i1:
   case MVT::i8:
     Opc = X86::MOV8rm;
case MVT::f64:
     } else {
       Opc = X86::LD_Fp64m;
       RC  = &X86::RFP64RegClass;
     }
     break;
   case MVT::f80:
     // No f80 support yet.
     return false;
+  case MVT::v4f32:
+    if (Aligned)
+      Opc = Subtarget->hasAVX() ? X86::VMOVAPSrm : X86::MOVAPSrm;
+    else
+      Opc = Subtarget->hasAVX() ? X86::VMOVUPSrm : X86::MOVUPSrm;
+    RC  = &X86::VR128RegClass;
+    break;
+  case MVT::v2f64:
+    if (Aligned)
+      Opc = Subtarget->hasAVX() ? X86::VMOVAPDrm : X86::MOVAPDrm;
+    else
+      Opc = Subtarget->hasAVX() ? X86::VMOVUPDrm : X86::MOVUPDrm;
+    RC  = &X86::VR128RegClass;
+    break;
+  case MVT::v4i32:
+  case MVT::v2i64:
+  case MVT::v8i16:
+  case MVT::v16i8:
+    if (Aligned)
+      Opc = Subtarget->hasAVX() ? X86::VMOVDQArm : X86::MOVDQArm;
+    else
+      Opc = Subtarget->hasAVX() ? X86::VMOVDQUrm : X86::MOVDQUrm;
+    RC  = &X86::VR128RegClass;
+    break;
   }
 
   ResultReg = createResultReg(RC);
   MachineInstrBuilder MIB =
     BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc), ResultReg);
   addFullAddress(MIB, AM);
   if (MMO)
     MIB->addMemOperand(*FuncInfo.MF, MMO);
if (!isTypeLegal(LI->getType(), VT, /*AllowI1=*/true))
     return false;
 
   const Value *Ptr = LI->getPointerOperand();
 
   X86AddressMode AM;
   if (!X86SelectAddress(Ptr, AM))
     return false;
 
+  unsigned Alignment = LI->getAlignment();
+  unsigned ABIAlignment = DL.getABITypeAlignment(LI->getType());
+  if (Alignment == 0) // Ensure that codegen never sees alignment 0
+    Alignment = ABIAlignment;
+  bool Aligned = Alignment >= ABIAlignment;
+
   unsigned ResultReg = 0;
-  if (!X86FastEmitLoad(VT, AM, createMachineMemOperandFor(LI), ResultReg))
+  if (!X86FastEmitLoad(VT, AM, createMachineMemOperandFor(LI), ResultReg,
+                       Aligned))
     return false;
 
   updateValueMap(I, ResultReg);
   return true;
 }
 
 static unsigned X86ChooseCmpOpcode(EVT VT, const X86Subtarget *Subtarget) {
   bool HasAVX = Subtarget->hasAVX();

test/CodeGen/X86/fast-isel-vecload.ll

+; RUN: llc -O0 -fast-isel -fast-isel-abort=1 -mtriple=x86_64-unknown-unknown -mattr=+sse2 < %s | FileCheck %s --check-prefix=SSE --check-prefix=ALL
+; RUN: llc -O0 -fast-isel -fast-isel-abort=1 -mtriple=x86_64-unknown-unknown -mattr=+avx < %s | FileCheck %s --check-prefix=AVX --check-prefix=ALL
+
+; Verify that fast-isel knows how to select aligned/unaligned vector loads.
+; Also verify that the selected load instruction is in the correct domain.
+
+define <16 x i8> @test_v16i8(<16 x i8>* %V) {
+; ALL-LABEL: test_v16i8:
+; SSE: movdqa  (%rdi), %xmm0
+; AVX: vmovdqa  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <16 x i8>, <16 x i8>* %V, align 16
+  ret <16 x i8> %0
+}
+
+define <8 x i16> @test_v8i16(<8 x i16>* %V) {
+; ALL-LABEL: test_v8i16:
+; SSE: movdqa  (%rdi), %xmm0
+; AVX: vmovdqa  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <8 x i16>, <8 x i16>* %V, align 16
+  ret <8 x i16> %0
+}
+
+define <4 x i32> @test_v4i32(<4 x i32>* %V) {
+; ALL-LABEL: test_v4i32:
+; SSE: movdqa  (%rdi), %xmm0
+; AVX: vmovdqa  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <4 x i32>, <4 x i32>* %V, align 16
+  ret <4 x i32> %0
+}
+
+define <2 x i64> @test_v2i64(<2 x i64>* %V) {
+; ALL-LABEL: test_v2i64:
+; SSE: movdqa  (%rdi), %xmm0
+; AVX: vmovdqa  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <2 x i64>, <2 x i64>* %V, align 16
+  ret <2 x i64> %0
+}
+
+define <16 x i8> @test_v16i8_unaligned(<16 x i8>* %V) {
+; ALL-LABEL: test_v16i8_unaligned:
+; SSE: movdqu  (%rdi), %xmm0
+; AVX: vmovdqu  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <16 x i8>, <16 x i8>* %V, align 4
+  ret <16 x i8> %0
+}
+
+define <8 x i16> @test_v8i16_unaligned(<8 x i16>* %V) {
+; ALL-LABEL: test_v8i16_unaligned:
+; SSE: movdqu  (%rdi), %xmm0
+; AVX: vmovdqu  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <8 x i16>, <8 x i16>* %V, align 4
+  ret <8 x i16> %0
+}
+
+define <4 x i32> @test_v4i32_unaligned(<4 x i32>* %V) {
+; ALL-LABEL: test_v4i32_unaligned:
+; SSE: movdqu  (%rdi), %xmm0
+; AVX: vmovdqu  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <4 x i32>, <4 x i32>* %V, align 4
+  ret <4 x i32> %0
+}
+
+define <2 x i64> @test_v2i64_unaligned(<2 x i64>* %V) {
+; ALL-LABEL: test_v2i64_unaligned:
+; SSE: movdqu  (%rdi), %xmm0
+; AVX: vmovdqu  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <2 x i64>, <2 x i64>* %V, align 4
+  ret <2 x i64> %0
+}
+
+define <4 x float> @test_v4f32(<4 x float>* %V) {
+; ALL-LABEL: test_v4f32:
+; SSE: movaps  (%rdi), %xmm0
+; AVX: vmovaps  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <4 x float>, <4 x float>* %V, align 16
+  ret <4 x float> %0
+}
+
+define <2 x double> @test_v2f64(<2 x double>* %V) {
+; ALL-LABEL: test_v2f64:
+; SSE: movapd  (%rdi), %xmm0
+; AVX: vmovapd  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <2 x double>, <2 x double>* %V, align 16
+  ret <2 x double> %0
+}
+
+define <4 x float> @test_v4f32_unaligned(<4 x float>* %V) {
+; ALL-LABEL: test_v4f32_unaligned:
+; SSE: movups  (%rdi), %xmm0
+; AVX: vmovups  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <4 x float>, <4 x float>* %V, align 4
+  ret <4 x float> %0
+}
+
+define <2 x double> @test_v2f64_unaligned(<2 x double>* %V) {
+; ALL-LABEL: test_v2f64_unaligned:
+; SSE: movupd  (%rdi), %xmm0
+; AVX: vmovupd  (%rdi), %xmm0
+; ALL-NEXT: retq
+entry:
+  %0 = load <2 x double>, <2 x double>* %V, align 4
+  ret <2 x double> %0
+}