This is an archive of the discontinued LLVM Phabricator instance.

[LV] Widen freeze instead of scalarizing it
ClosedPublic

Authored by lizhijin on May 5 2022, 8:28 AM.

Details

Summary

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

Fixes #54992

Diff Detail

Event Timeline

lizhijin created this revision.May 5 2022, 8:28 AM
lizhijin requested review of this revision.May 5 2022, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 8:28 AM
Allen added a subscriber: Allen.May 5 2022, 6:58 PM
Matt added a subscriber: Matt.May 6 2022, 1:48 PM

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 added a comment.May 9 2022, 6:25 AM

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 added a comment.May 9 2022, 6:27 AM

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.

lizhijin retitled this revision from [AArch64][SVE] Fix assertions when vectorizing Freeze Instructions to [LV] Widen freeze instead of scalarizing it.May 10 2022, 7:29 AM
lizhijin updated this revision to Diff 428373.May 10 2022, 7:31 AM
fhahn accepted this revision.May 12 2022, 10:55 AM

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.

This revision is now accepted and ready to land.May 12 2022, 10:55 AM
fhahn added inline comments.May 12 2022, 10:56 AM
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

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?

mdchen added a subscriber: mdchen.May 12 2022, 8:26 PM
lizhijin edited the summary of this revision. (Show Details)May 13 2022, 12:42 AM
lizhijin updated this revision to Diff 429751.May 16 2022, 9:47 AM
peterwaller-arm added inline comments.
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
}
lizhijin updated this revision to Diff 430441.May 18 2022, 10:25 AM
This revision was landed with ongoing or failed builds.May 18 2022, 9:29 PM
This revision was automatically updated to reflect the committed changes.