[PATCH] D48847: [GISel]: Add Legalization/lowering code for bit counting operations
Roman Tereshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 14 11:19:43 PDT 2018
rtereshin added a comment.
Hi Aditya,
Thank you for doing this!
Most of my comments are nitpicks and mild suggestions except the following ones:
https://reviews.llvm.org/D48847#inline-446232
https://reviews.llvm.org/D48847#inline-446277
https://reviews.llvm.org/D48847#inline-446317
https://reviews.llvm.org/D48847#inline-446328
================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:671
+ return buildICmp(Pred, getDestFromArg(Dst), getRegFromArg(UseArgs)...);
+ }
----------------
I'm not sure what's the point of having variadic templates here. Does `buildICmp` or `buildSelect` make sense with 0, 1, or let's say 4 operands (excluding destination)?
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:40
+ MIRBuilder.setMF(MF);
+}
LegalizerHelper::LegalizeResult
----------------
👍
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1063
+ auto MIBCtlzZU =
+ MIRBuilder.buildInstr(TargetOpcode::G_CTLZ_ZERO_UNDEF, Ty, SrcReg);
+ auto MIBZero = MIRBuilder.buildConstant(Ty, 0);
----------------
Do you think we could `MI.setDesc(TII.get(TargetOpcode:: G_CTLZ_ZERO_UNDEF));` here in-place instead of recreating the instruction from scratch?
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1083
+ unsigned Op = SrcReg;
+ for (unsigned i = 0; (1U << i) <= (Len / 2); ++i) {
+ auto MIBTmp3 = MIRBuilder.buildConstant(Ty, 1ULL << i);
----------------
I don't think this is going to work if `Len` is not a power of 2.
For the sake of a smaller example, let's say `Len` is 6. The shift amount is going to take values 1 and 2. Let's say the `SrcReg`'s value is `10 00 00`. The `Op` takes values:
`10 00 00`
`11 00 00` = `10 00 00` | `01 00 00`, `i == 0`
`11 11 00` = `11 00 00` | `00 11 00`, `i == 1`
The final value is 2, while it had to be 0.
This could probably be fixed by rounding `Len` up to the closest power of 2.
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1084
+ for (unsigned i = 0; (1U << i) <= (Len / 2); ++i) {
+ auto MIBTmp3 = MIRBuilder.buildConstant(Ty, 1ULL << i);
+ auto MIBOp = MIRBuilder.buildInstr(
----------------
Maybe slightly more meaningful name?
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1093
+ MIRBuilder.buildInstr(TargetOpcode::G_CTPOP, MI.getOperand(0).getReg(),
+ MIBNot);
+ MI.eraseFromParent();
----------------
Same here, could we change `MI` in-place?
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1101
+ MIRBuilder.recordInsertion(&MI);
+ return Legalized;
+ }
----------------
I think there is a little opportunity here, not sure how useful in practice: if we check
```
if (!isLegalOrCustom({TargetOpcode::G_CTPOP, {Ty}}) &&
isLegalOrCustom({TargetOpcode:: G_CTLZ_ZERO_UNDEF, {Ty}}))
```
here and fall-through if `true` we could lower this to `32 - ctlz_zero_undef(~x & (x-1))` with no extra `select`s.
Just a thought, it almost certainly doesn't worth the effort.
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1129
+ MIRBuilder.buildInstr(TargetOpcode::G_SUB, Ty, SrcReg,
+ MIRBuilder.buildConstant(Ty, 1)));
+ if (!isLegalOrCustom({TargetOpcode::G_CTPOP, {Ty}}) &&
----------------
We already have -1 constant built just above, maybe do `G_ADD` with it instead?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:32
+ auto CheckStr = R"(
+ CHECK: [[CZU:%[0-9]+]]:_(s64) = G_CTTZ_ZERO_UNDEF
+ CHECK: [[ZERO:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
----------------
missing `%0` at the end?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:139
+ auto CheckStr = R"(
+ CHECK: [[CZU:%[0-9]+]]:_(s64) = G_CTLZ_ZERO_UNDEF
+ CHECK: [[ZERO:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
----------------
ditto
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:185
+ CHECK: G_XOR
+ CHECK: G_CTPOP
+ )";
----------------
Maybe it makes sense to check here that we actually thread things correctly.
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:74
+ M->setDataLayout(TM.createDataLayout());
+
+ if (MIR->parseMachineFunctions(*M, MMI))
----------------
Shouldn't we call `MachineModuleInfo::doInitialization(Module &M)` here?
Also, if we do, MMI will be accessible as `MMI.getModule()` which should make `createDummyModule`s interface simpler.
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:95
+body: |
+ bb.1:
+ %0(s64) = COPY $x0
----------------
Why not `bb.0`?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:98
+ %1(s64) = COPY $x1
+ %2(s64) = COPY $x2
+)MIR") + Twine(MIRFunc) + Twine("...\n"))
----------------
Do you want to make these live-in?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:99
+ %2(s64) = COPY $x2
+)MIR") + Twine(MIRFunc) + Twine("...\n"))
+ .toNullTerminatedStringRef(S);
----------------
The last two `Twine(...)`s are probably not needed (I would expect `Twine::operator+` to be overloaded for every string type)
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:101
+ .toNullTerminatedStringRef(S);
+ std::unique_ptr<MIRParser> MIR;
+ auto MMI = make_unique<MachineModuleInfo>(&TM);
----------------
Any reason this is created here and passed to `parseMIR` by reference instead of being created and destroyed all within `parseMIR`?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:109
+static MachineFunction *getMFFromMMI(const Module *M,
+ const MachineModuleInfo *MMI) {
+ Function *F = M->getFunction("func");
----------------
It looks like if `MMI` is initialized properly the `Module *M` will be accessible from `MMI`.
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:116
+static void collectCopies(SmallVectorImpl<unsigned> &Copies,
+ MachineFunction *MF) {
+ for (auto &MBB : *MF)
----------------
Maybe swap these so an out parameter is the last?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:144
+ SmallVector<unsigned, 4> Copies;
+ MachineBasicBlock *EntryMBB;
+ MachineIRBuilder B;
----------------
doesn't look like this is used by any of the tests at the moment.
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:146
+ MachineIRBuilder B;
+ MachineRegisterInfo *MRI;
+};
----------------
Not used by any of the tests?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:147
+ MachineRegisterInfo *MRI;
+};
+
----------------
So we run multiple tests over the same instance of machine function, adding new instructions w/o clearing out the machine function and the rest of the context? Also, in some not really well defined order probably (like alphabetical by test name or something).
Shouldn't we put all of this into `setUp` instead and clear it out properly in `tearDown` and run every test in a clean state?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:162
+ (void)s64; \
+ x; \
+ computeTables(); \
----------------
Maybe call `x` a `Block` or `SettingUpActionsBlock` or something along these lines, then
```
do Block while (0);
```
here and at the "call site" something like
```
LInfoBuilder(A, {
getActionDefinitionsBuilder(G_CTTZ_ZERO_UNDEF).legalFor({s64});
});
```
instead of
```
LInfoBuilder(A,
getActionDefinitionsBuilder(G_CTTZ_ZERO_UNDEF).legalFor({s64}););
```
?
As for the name, maybe `DefineLegalizerInfo` is a tad better than `LInfoBuilder`, not sure.
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:164
+ computeTables(); \
+ } \
+ };
----------------
Maybe also call `verify(*ST.getInstrInfo());`?
================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:188
+ return FC.CheckInput(SM, OutBuffer, CheckStrings);
+}
----------------
Do we run MachineVerifier somewhere here?
https://reviews.llvm.org/D48847
More information about the llvm-commits
mailing list