paulwalker-arm added inline comments.
================ Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.h:298-299 + bool LittleEndian, + unsigned MinSVEVectorSizeInBitsOverride = 0, + unsigned MaxSVEVectorSizeInBitsOverride = 0); ---------------- Out of interest are these defaults ever relied upon? ================ Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:380-386 + assert(MinSVEVectorSize % 128 == 0 && + "SVE requires vector length in multiples of 128!"); + assert(MaxSVEVectorSize % 128 == 0 && + "SVE requires vector length in multiples of 128!"); + assert((MaxSVEVectorSize >= MinSVEVectorSize || + MaxSVEVectorSize == 0) && + "Minimum SVE vector size should not be larger than its maximum!"); ---------------- These asserts are fine but you'll see from the original implementations of `getM..SVEVectorSizeInBits` that I do not rely on the user passing the correct values. Instead I also always process the sizes to ensure the values of `MinSVEVectorSize` and `MaxSVEVectorSize`are sane. Can you do likewise here? ================ Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:357 + Attribute VScaleRangeAttr = F.getFnAttribute(Attribute::VScaleRange); + if (VScaleRangeAttr.isValid()) + std::tie(MinSVEVectorSize, MaxSVEVectorSize) = ---------------- bsmith wrote: > paulwalker-arm wrote: > > I don't know if this is possible but I feel we need a `HasSVE` like check > > here? > I'm not sure this is really doable here without picking apart the feature > string, I think it makes more sense to just set the values and assert when > using the accessors without SVE enabled. Fair enough. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103702/new/ https://reviews.llvm.org/D103702 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
