As is mentioned above
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Hi @junparser, the patch looks sensible to me! I just had a couple of minor comments if that's ok.
| llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
|---|---|---|
| 658 | nit: Do you need to fix the clang-tidy warning here? | |
| 662 | Could you potentially fold these two cases into one somehow? Maybe with a switch-case statement? I'm just imagining a situation where we might want other patterns too like vl32, vl64, etc. | |
| llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
|---|---|---|
| 662 | There is no other special pattern except vl16. But I do think switch-case is more straightforward | |
| llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
|---|---|---|
| 662 | OK, thanks for making this a switch statement. I was just thinking that in the developer manual we say we also support vl1-vl256 so at some point we may add more enums in LLVM too. | |
| 671 | I was actually wondering if we could commonise this code somehow. Perhaps by setting a MinNumElts variable in the case statements, i.e. unsigned MinNumElts;
...
case AArch64SVEPredPattern::vl8:
MinNumElts = Pattern;
break;
case AArch64SVEPredPattern::vl16:
MinNumElts = 16;
break;
}
if (NumElts < MinNumElts) return None;
return Optional<Instruction *>(IC.replaceInstUsesWith(
II, ConstantInt::get(II.getType(), NumElts))); | |
nit: Do you need to fix the clang-tidy warning here?