[PATCH] D48847: [GISel]: Add Legalization/lowering code for bit counting operations

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked 10 inline comments as done.
aditya_nandakumar added a comment.

Thanks roman for the feedback. I'll update the patch shortly based on the feedback.



================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:671
+    return buildICmp(Pred, getDestFromArg(Dst), getRegFromArg(UseArgs)...);
+  }
 
----------------
rtereshin wrote:
> 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)?
This required less typing one extra argument ;) . Additionally this lets you directly buildICmp like this
auto MIB = something...
auto Reg = someReg;
auto MIBCmp = Builder.buildICmp(Pred, SomeTy, MIB, Reg); This lets Op0 and Op1 be either MachineInstrBuilder or Register (for the various 4 combinations possible).
If you need to explicitly specify the use args, it needs to be something 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.


================
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);
----------------
rtereshin wrote:
> Do you think we could `MI.setDesc(TII.get(TargetOpcode:: G_CTLZ_ZERO_UNDEF));` here in-place instead of recreating the instruction from scratch?
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.


================
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);
----------------
rtereshin wrote:
> 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.
Good catch. I'll replace it with the following.
NewLen = RoundUpToPow2(Len);
x = x | (x >>1);
... until NewLen/2
return Len - PopCount(x);



================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1101
+    MIRBuilder.recordInsertion(&MI);
+    return Legalized;
+  }
----------------
rtereshin wrote:
> 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.
That's something we could do. I can possibly address this in a subsequent patch.


================
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}}) &&
----------------
rtereshin wrote:
> We already have -1 constant built just above, maybe do `G_ADD` with it instead?
I've changed it. Thanks


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:74
+  M->setDataLayout(TM.createDataLayout());
+
+  if (MIR->parseMachineFunctions(*M, MMI))
----------------
rtereshin wrote:
> 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.
This was copied over from another unit test. I'll refactor both of these in a subsequent patch.


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:116
+static void collectCopies(SmallVectorImpl<unsigned> &Copies,
+                          MachineFunction *MF) {
+  for (auto &MBB : *MF)
----------------
rtereshin wrote:
> Maybe swap these so an out parameter is the last?
Ditto for all of the above.


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:144
+  SmallVector<unsigned, 4> Copies;
+  MachineBasicBlock *EntryMBB;
+  MachineIRBuilder B;
----------------
rtereshin wrote:
> doesn't look like this is used by any of the tests at the moment.
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.


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:146
+  MachineIRBuilder B;
+  MachineRegisterInfo *MRI;
+};
----------------
rtereshin wrote:
> Not used by any of the tests?
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.


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:147
+  MachineRegisterInfo *MRI;
+};
+
----------------
rtereshin wrote:
> 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?
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.
I hope I've not misunderstood your question.


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:188
+  return FC.CheckInput(SM, OutBuffer, CheckStrings);
+}
----------------
rtereshin wrote:
> Do we run MachineVerifier somewhere here?
No we don't currently run the verifier here.


https://reviews.llvm.org/D48847