[LV] Widen freeze instead of scalarizing it

[lizhijin]

Summary

This patch changes the strategy for vectorizing freeze instrucion, from replicating multiple times to widening according to selected VF.

Fixes #54992

[david-arm]

Hi @lizhijin, thanks a lot for the fix. This looks a lot better than the previous one! I just had a few minor comments ...

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9335

Maybe the Freeze instruction is better in it's own case statement at the bottom? i.e. something like

case Freeze: {
  State.ILV->setDebugLocFromInst(CI);

  for (unsigned Part = 0; Part < State.UF; ++Part) {
    SmallVector<Value *, 2> Ops;
    for (VPValue *VPOp : operands())
      Ops.push_back(State.get(VPOp, Part));

      Value *Freeze = Builder.CreateFreeze(Ops[0]);
      State.set(this, Freeze, Part);
      State.ILV->addMetadata(Cast, &I);
    }
  }
  break;
}

This avoids you having to check for the Freeze opcode as a special case in the loop below.

llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll

2 ↗

(On Diff #427331)

I think you can remove the -mtriple aarch64-linux-gnu from the RUN line here because it has already been specified below with the target triple line.

7 ↗

(On Diff #427331)

Is it possible to remove these extra function attributes at the end?

42 ↗

(On Diff #427331)

I think you can remove both of these attribute lists

[fhahn]

Widening freeze looks good, Alive2 also agrees https://alive2.llvm.org/ce/z/P3vnNs

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9335

I think the above can be simplified, freeze must have a single operand so

SmallVector<Value *, 2> Ops;
for (VPValue *VPOp : operands())
  Ops.push_back(State.get(VPOp, Part));

should not be needed.

freeze also cannot have metadata, so State.ILV->addMetadata(Cast, &I); should not be needed.

llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll

21 ↗

(On Diff #427331)

It's probably best to avoid undef here, as it makes the whole test have UB. This for example means that automatic verification with alive2 won't work (if the source has UB , any transformation will be verified as valid).

It simpler to update the GEP to just use a i8* and pass it in as a function argument.

25 ↗

(On Diff #427331)

nit: block and value names could be shortened a bit to make the test a bit easier to read.

[fhahn]

Also the fix is not AArch64/SVE specific, so the title/description of the patch should probably be updated to reflect this. It could say something like [LV] Widen freeze instead of scalarizing it.. It would probably also be good to have a target independent test.

[fhahn]

LGTM, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9368

nit: no need for newline here.

llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll

20 ↗

(On Diff #428373)

Might be good to use a real condition here to make the test more robust with respect to future changes.

[fhahn]

llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll

25 ↗

(On Diff #427331)

would still be good to improve the naming a bit, possibly also dropping the indvars. prefix from names

[fhahn]

IIUC this fixes https://github.com/llvm/llvm-project/issues/54992. Could you add Fixes #54992 to the commit message to auto-close the issue when the patch lands?

[peterwaller-arm]

llvm/test/Transforms/LoopVectorize/vector-freeze.ll

37

Thanks for the patch. Here is a suggestion for a smaller test case (which I produced before spotting your patch, and your patch does fix, thanks!)

define i64 @test(ptr noalias readonly %addr) {
; CHECK-LABEL: @test(
; CHECK:       vector.body:
; CHECK:       freeze <16 x i64>

; SVE-LABEL: @test(
; SVE:       vector.body:
; SVE:       freeze <vscale x 16 x i64>

entry:
  br label %loop

exit:
  ret i64 %tmp4

loop:
  %tmp3 = phi ptr [ %tmp6, %loop ], [ %addr, %entry ]
  %tmp4 = freeze i64 0
  %tmp5 = add i64 0, 0
  %tmp6 = getelementptr inbounds ptr, ptr %tmp3, i64 1
  %tmp7 = icmp eq ptr %tmp6, null
  br i1 %tmp7, label %exit, label %loop
}

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

auto IsVectorizableOpcode = [](unsigned Opcode) {
     case Instruction::SRem:
     case Instruction::Sub:
     case Instruction::Trunc:
     case Instruction::UDiv:
     case Instruction::UIToFP:
     case Instruction::URem:
     case Instruction::Xor:
     case Instruction::ZExt:
+    case Instruction::Freeze:
       return true;
     }
     return false;
   };
 
   if (!IsVectorizableOpcode(I->getOpcode()))
     return nullptr;
 
void VPWidenRecipe::execute(VPTransformState &State) {
   case Instruction::FDiv:
   case Instruction::FRem:
   case Instruction::Shl:
   case Instruction::LShr:
   case Instruction::AShr:
   case Instruction::And:
   case Instruction::Or:
   case Instruction::Xor: {
     // Just widen unops and binops.
     State.ILV->setDebugLocFromInst(&I);
 
     for (unsigned Part = 0; Part < State.UF; ++Part) {
       SmallVector<Value *, 2> Ops;
       for (VPValue *VPOp : operands())
         Ops.push_back(State.get(VPOp, Part));
 
       Value *V = Builder.CreateNAryOp(I.getOpcode(), Ops);
for (unsigned Part = 0; Part < State.UF; ++Part) {
 
       // Use this vector value for all users of the original instruction.
       State.set(this, V, Part);
       State.ILV->addMetadata(V, &I);
     }
 
     break;
   }
+  case Instruction::Freeze: {
+    State.ILV->setDebugLocFromInst(&I);
+
+    for (unsigned Part = 0; Part < State.UF; ++Part) {
+      Value *Op = State.get(getOperand(0), Part);
+      Value *Freeze = Builder.CreateFreeze(Op);
+      State.set(this, Freeze, Part);
+    }
+    break;
+  }
   case Instruction::ICmp:
   case Instruction::FCmp: {
     // Widen compares. Generate vector compares.
     bool FCmp = (I.getOpcode() == Instruction::FCmp);
     auto *Cmp = cast<CmpInst>(&I);
     State.ILV->setDebugLocFromInst(Cmp);
     for (unsigned Part = 0; Part < State.UF; ++Part) {
       Value *A = State.get(getOperand(0), Part);

llvm/test/Transforms/LoopVectorize/vector-freeze.ll

+; RUN: opt -loop-vectorize -force-vector-width=16 -force-vector-interleave=1 -S < %s | FileCheck %s
+; RUN: opt -loop-vectorize -scalable-vectorization=on -force-target-supports-scalable-vectors=true -force-vector-width=16 -force-vector-interleave=1 -S < %s | FileCheck %s --check-prefix=SVE
+
+define void @test(i8* noalias readonly %addr, i1 %cond) {
+; CHECK-LABEL: @test(
+; CHECK:       vector.body:
+; CHECK:       freeze <16 x i8>
+
+; SVE-LABEL: @test(
+; SVE:       vector.body:
+; SVE:       freeze <vscale x 16 x i8>
+
+for.body.preheader:
+  br label %for.body
+
+for.body:                                         ; preds = %if.end, %for.body.preheader
+  %iv = phi i64 [ 0, %for.body.preheader ], [ %iv.next, %if.end ]
+  %arrayidx = getelementptr inbounds i8, i8* %addr, i64 %iv
+  %0 = load i8, i8* %arrayidx, align 1
+  br i1 %cond, label %if.end, label %lor.lhs.false
+
+lor.lhs.false:                                    ; preds = %for.body
+  %.fr = freeze i8 %0
+  %cmp = icmp eq i8 %.fr, 0
+  br i1 %cmp, label %if.end, label %switch.early.test
+
+switch.early.test:                                ; preds = %lor.lhs.false
+  br label %if.end
+
+if.end:                                           ; preds = %switch.early.test, %lor.lhs.false, %for.body
+  %iv.next = add nuw nsw i64 %iv, 1
+  %exitcond.not = icmp eq i64 %iv.next, 0
+  br i1 %exitcond.not, label %for.end.loopexit, label %for.body
+
+for.end.loopexit:                                 ; preds = %if.end
+  ret void
+}