[PATCH] D56957: GlobalISel: Handle some odd splits in fewerElementsVector

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1398
+    for (unsigned Offset = 0; Offset < BitsForNumParts; Offset += NarrowSize) {
+      SmallVector<SrcOp, 4> SrcOps;
+      for (unsigned I = 1, E = MI.getNumOperands(); I != E; ++I) {
----------------
arsenm wrote:
> dsanders wrote:
> > arsenm wrote:
> > > dsanders wrote:
> > > > Hi Matt,
> > > > 
> > > > I just came across the various uses of SrcOp in LegalizerHelper and thought I should mention that this type wasn't really meant to be used outside of MachineIRBuilder and its specializations. It's a bit of machinery to be able to factor out the common overloads.
> > > > 
> > > > What was the reason for introducing the SrcOp variables? Does MachineOperand or registers achieve the same end result?
> > > I think this was because you can't implicitly convert from SmallVector<unsigned> to ArrayRef<SrcOp> for the final use
> > That makes sense, we should be able to add a conversion for that. In later commits there's also some SrcOp variables that aren't in a container. What was the reason for those?
> In the narrowScalarShiftByConstant for example, I think this was to have an uninitialized variable that the later build calls would set from the MachineInstrBuilder results. It would be a pain if this was a register value, since you don't really want to be referring to registers at all.
Thanks. Given that's the aim, we could probably replace the ones like that with a default-constructed MachineInstrBuilder since that's what the SrcOp there is really holding for its lifetime (except the initial 'uninitialized' portion where it's holding register 0).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56957/new/

https://reviews.llvm.org/D56957