[PATCH] D125016: [AArch64][SVE] Fix assertions when vectorizing Freeze Instructions
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 9 06:25:03 PDT 2022
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
More information about the llvm-commits
mailing list