[PATCH] D30529: [RFC][GlobalISel] Enable legalizing non-power-of-2 sized types.
Tim Northover via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 25 15:59:01 PDT 2017
t.p.northover added a comment.
Hi Kristof,
Thanks for working on this and I'm really sorry it took so long to reply.
I like the basic structure and I think it should be able to represent everything we need. I originally thought that mixing the strategies with `setAction` calls was clunky, but I assume almost all of that is going to go away once sensible default strategies are in place for all the operations?
================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:45-46
- DefaultActions[TargetOpcode::G_ADD] = NarrowScalar;
- DefaultActions[TargetOpcode::G_LOAD] = NarrowScalar;
- DefaultActions[TargetOpcode::G_STORE] = NarrowScalar;
+ setLegalizeScalarToDifferentSizeStrategy(
+ TargetOpcode::G_ADD, 0, narrowToSmallerAndUnsupportedIfTooSmall);
+ setLegalizeScalarToDifferentSizeStrategy(
----------------
This should probably be widen-then-narrow, but I assume it's like this to minimize the functional diff.
================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:81-89
+ SizeAndActionsVec &v =
+ Type.isPointer()
+ ? AddressSpace2SpecifiedActions[Type.getAddressSpace()]
+ : Type.isVector()
+ ? ElemSize2SpecifiedActions[Type.getElementType()
+ .getSizeInBits()]
+
----------------
Since v is only used for the push_back maybe just do it directly with ifs:
auto SizeAction = std::make_pair(Type.getSizeInBits(), Action);
if (Type.isPointer())
AddressSpace2SpecifiedActions[Type.getAddressSpace()].push_back(SizeAction);
else if (Type.isVector())
ElemSize2SpecifiedActions[Type.getElementType().getSizeInBits()].push_back(SizeAction);
else
ScalarSpecifiedActions.push_back(SizeAction);
================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:253-260
+ int VecIdx = -1;
+ for (int i = Vec.size() - 1; i >= 0; --i) {
+ const uint32_t BitSize = Vec[i].first;
+ if (BitSize <= Size) {
+ VecIdx = i;
+ break;
+ }
----------------
I think this is approximately
VecIdx = std::lower_bound(Vec.begin(), Vec.end()) - Vec.begin();
which has added binary-search goodness.
================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:275-277
+ // FIXME: do we really need a loop here? Or could we assert at construction
+ // time that there cannot be 2 consecutive actions in a SizeAndActionsVec
+ // for which NeedsLegalizingToDifferentSize() is true.
----------------
I think the assertion would be reasonable.
================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:107-108
+
+static LegalizerInfo::SizeAndActionsVec
+widen_1_8_16_narrowLarger(const LegalizerInfo::SizeAndActionsVec &v) {
+ assert(v.size() >= 1);
----------------
I think this is the same as `widen_1_8_16_narrowToLargest` isn't it?
https://reviews.llvm.org/D30529
More information about the llvm-commits
mailing list