[PATCH] D125016: [AArch64][SVE] Fix assertions when vectorizing Freeze Instructions

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9330
+  case Instruction::Xor:
+  case Instruction::Freeze: {
     // Just widen unops and binops.
----------------
david-arm wrote:
> 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.
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.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll:21
+  %indvars.iv132 = phi i64 [ 0, %for.body.preheader ], [ %indvars.iv.next133, %if.end ]
+  %arrayidx = getelementptr inbounds [1 x i8], [1 x i8]* undef, i64 0, i64 %indvars.iv132
+  %0 = load i8, i8* %arrayidx, align 1
----------------
 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.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll:25
+
+lor.lhs.false:                                    ; preds = %for.body
+  %.fr122 = freeze i8 %0
----------------
nit: block and value names could be shortened a bit to make the test a bit easier to read.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125016/new/

https://reviews.llvm.org/D125016