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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

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



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


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -loop-vectorize -mtriple aarch64-linux-gnu -S < %s | FileCheck %s
+; RUN: opt -loop-vectorize -mattr=+sve  -mtriple aarch64-linux-gnu -S < %s | FileCheck %s --check-prefix=SVE
----------------
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.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll:7
+
+define void @test() #1 personality i8* undef {
+; CHECK-LABEL: @test(
----------------
Is it possible to remove these extra function attributes at the end?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/vector-freeze.ll:42
+
+attributes #0 = { argmemonly nofree nosync nounwind willreturn }
+attributes #1 = { "target-features"="+aes,+crypto,+neon,+sha2,+v8a" }
----------------
I think you can remove both of these attribute lists


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

https://reviews.llvm.org/D125016