[PATCH] D80615: [CodeGen] Fix warnings in getPackedVectorTypeFromPredicateType
Francesco Petrogalli via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 29 12:00:38 PDT 2020
fpetrogalli requested changes to this revision.
fpetrogalli added a comment.
This revision now requires changes to proceed.
HI @david-arm ,
I will not hold this patch if you disagree on the nit, but I think you should remove the tests as they seem to duplicate something that is already tested.
Ciao,
Francesco
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4626-4629
+ ElementCount EC = PredVT.getVectorElementCount();
- if (NumElts != 2 && NumElts != 4 && NumElts != 8 && NumElts != 16)
+ if (EC.Min != 2 && EC.Min != 4 && EC.Min != 8 && EC.Min != 16)
return EVT();
----------------
NIT:
We could be explicit here on the PredVT that we want to use and swap the statements as follows:
```
if (PredVT != MVT::nxv16i1 && PredVT != MVT::nxv8i1 && ...)
return EVT();
ElementCount EC = ...
```
I am also wondering whether we can remove at all the idea of using EC.Min in this code and just use TypeSize. Something like the following:
```
if (PredVT != MVT::nxv16i1 && PredVT != MVT::nxv8i1 && ...)
return EVT();
// This could actually be a global in the AArch64 namespace, to be used as AArch64::SVEBits,
// I suspect it will be handy in other situations.
const auto SVEVecSize = TypeSize::Scalable(AArch64::SVEBitsPerBlock);
EVT MemVT = PredVT;
while (MemVT.getSizeInBits() < SVETySy)
MemVT = MemVT.widenIntegerVectorElementType(Ctx);
return MemVT;
```
Seems like a good thing to get rid of the EC.Min here?
================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-prfw.ll:1
+; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s | FileCheck %s
+
----------------
These are already tested here: https://github.com/llvm/llvm-project/blob/master/llvm/test/CodeGen/AArch64/sve-intrinsics-contiguous-prefetches.ll
Any reason for re-adding them in a separate file? Or am I missing something?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80615/new/
https://reviews.llvm.org/D80615
More information about the llvm-commits
mailing list