Ported Legalization expansions for CTLZ/CTTZ.. from DAG to GISel.
Diff Detail
Event Timeline
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
include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h | ||
---|---|---|
671 | 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)? | |
lib/CodeGen/GlobalISel/LegalizerHelper.cpp | ||
40 | ๐ | |
1063 | Do you think we could MI.setDesc(TII.get(TargetOpcode:: G_CTLZ_ZERO_UNDEF)); here in-place instead of recreating the instruction from scratch? | |
1083 | 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: 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. | |
1084 | Maybe slightly more meaningful name? | |
1093 | Same here, could we change MI in-place? | |
1101 | 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 selects. Just a thought, it almost certainly doesn't worth the effort. | |
1129 | We already have -1 constant built just above, maybe do G_ADD with it instead? | |
unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp | ||
33 | missing %0 at the end? | |
140 | ditto | |
186 | Maybe it makes sense to check here that we actually thread things correctly. | |
unittests/CodeGen/GlobalISel/LegalizerHelperTest.h | ||
75 | Shouldn't we call MachineModuleInfo::doInitialization(Module &M) here? Also, if we do, MMI will be accessible as MMI.getModule() which should make createDummyModules interface simpler. | |
96 | Why not bb.0? | |
99 | Do you want to make these live-in? | |
100 | The last two Twine(...)s are probably not needed (I would expect Twine::operator+ to be overloaded for every string type) | |
102 | Any reason this is created here and passed to parseMIR by reference instead of being created and destroyed all within parseMIR? | |
110 | It looks like if MMI is initialized properly the Module *M will be accessible from MMI. | |
117 | Maybe swap these so an out parameter is the last? | |
145 | doesn't look like this is used by any of the tests at the moment. | |
147 | Not used by any of the tests? | |
148 | 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? | |
163 | 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. | |
165 | Maybe also call verify(*ST.getInstrInfo());? | |
189 | Do we run MachineVerifier somewhere here? |
Thanks roman for the feedback. I'll update the patch shortly based on the feedback.
include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h | ||
---|---|---|
671 | This required less typing one extra argument ;) . Additionally this lets you directly buildICmp like this template<typename DstTy, typename UseArg0Ty, typename UseArg1Ty> MachineInstrBuilder buildICmp(CmpInst::Predicate Pred, DstTy &&Dst, UseArg0Ty &&Arg0, UseArg1Ty &&Arg1) { return buildICmp(Pred, getDestFromArg(Dst), getRegFromArg(Arg0), getRegFromArg(Arg1)); } Which is a little more typing and longer. | |
lib/CodeGen/GlobalISel/LegalizerHelper.cpp | ||
1063 | Definitely we should be doing that as often as we can, but in this case, if we go that route, we'd end up in a situation where the final select would need to create a new vreg and then it's probably not safe to just blindly replace the original dest with new reg. It's definitely possible to do it in a more ugly way where we replace the first inst in place, change it's destination to a newly created vreg but this sacrifices readability of code. I think CTLZ is not frequent enough to do this. | |
1083 | Good catch. I'll replace it with the following. | |
1101 | That's something we could do. I can possibly address this in a subsequent patch. | |
1129 | I've changed it. Thanks | |
unittests/CodeGen/GlobalISel/LegalizerHelperTest.h | ||
75 | This was copied over from another unit test. I'll refactor both of these in a subsequent patch. | |
117 | Ditto for all of the above. | |
145 | This is currently only used to set the insertion point. I left it available if any of the future tests needed that. I can remove it if required. | |
147 | MRI was just left here in case any tests want to use it - but it's easily accessible with B.getMF().getRegInfo(). Again, I don't mind removing it. | |
148 | I believe each googletest does not reuse the same test fixture object across multiple tests - it creates a new fixture object, constructs/calls setup, runs test, and call teardown()/destructor. I believe each test in it's own should be in a clean state when it runs. | |
189 | No we don't currently run the verifier here. |
Thank you!
LGTM
unittests/CodeGen/GlobalISel/LegalizerHelperTest.h | ||
---|---|---|
148 | True, (https://github.com/google/googletest/blob/master/googletest/docs/primer.md, https://github.com/google/googletest/blob/master/googletest/docs/faq.md#should-i-use-the-constructordestructor-of-the-test-fixture-or-the-set-uptear-down-function), maybe I got it mixed up with the python version of the library. Thanks! | |
189 | Feels like a good idea to do that. Maybe, by default with the possibility to opt out on a test-case per test-case basis. |
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)?