[PATCH] D46018: [GlobalISel][IRTranslator] Split aggregates during IR translation

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added a comment.

Hi Amara,

Alright, I'm done with the review.

General notes:

1. there is insufficient tests coverage for cases like PHIs and nested / shared / repeated aggregate constants
2. there is code that is likely to be dead
3. pretty much every use-case of `computeValueLLTs` looks like a hack, tbh, maybe it could be helped if we actually start mapping `Type`s instead of `Value`s to offsets //and// LLTs.

None of it is a showstopper IMO though, please address what seems to be reasonable to get addressed in the initial patch and we will be good to go with this.

Thanks!
Roman



================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:146
+  for (unsigned i = 0; i < SplitTys.size(); ++i)
+    Regs->push_back(0);
+  return *Regs;
----------------
Just `Regs.resize(SplitTys.size())` may be a littler cleaner.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:921
+unsigned IRTranslator::packRegs(const Value &V,
+                                  MachineIRBuilder &MIRBuilder) {
+  ArrayRef<unsigned> Regs = getOrCreateVRegs(V);
----------------
Indentation is off


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:974
+    else
+      Res = getOrCreateVReg(CI);
+
----------------
```
    unsigned Res = IsSplitType
        ? MRI->createGenericVirtualRegister(getLLTForType(*CI.getType(), *DL))
        : getOrCreateVReg(CI);
```
may work a little nicer here.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1265
+  unsigned Val = getOrCreateVRegs(*U.getOperand(0))[0];
+  unsigned Idx = getOrCreateVRegs(*U.getOperand(1))[0];
   MIRBuilder.buildExtractVectorElement(Res, Val, Idx);
----------------
It looks like `getOrCreateVReg` (singular) is already implemented so it could be used here.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1318
+          MIB.addUse(ValRegs[i]);
+          MIB.addMBB(Pred);
+        }
----------------
There are two `i`s in scope here. Do we have a test covering this?


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1335
+  return SplitTys.size() > 1;
+}
+
----------------
This function is rather a hack, but I guess it's not a big deal.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1473
       continue; // Don't handle zero sized types.
-    VRegArgs.push_back(getOrCreateVReg(Arg));
+    if (VMap.contains(cast<Value>(Arg))) {
+      auto &VRegs = *VMap.getVRegs(cast<Value>(Arg));
----------------
I can't see how this condition could be true. And none of the tests fail if an assertion is inserted under this condition. If it is possible, could you please add a test? Thanks!


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1492
+  auto ArgIt = F.arg_begin();
+  for (unsigned i = 0; i < VRegArgs.size(); ++i) {
+    // If the argument is an unsplit scalar then don't use unpackRegs to avoid
----------------
could be replaced with range-based for.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1497
+      auto &VRegs = *VMap.getVRegs(cast<Value>(*ArgIt));
+      if (VRegs.empty())
+        VRegs.push_back(VRegArgs[i]);
----------------
How this could be not true? As above, an assertion in in `else` branch shows `VRegs` is always empty here as far as the existing tests' coverage go.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1501
+    } else {
+      unpackRegs(*ArgIt++, VRegArgs[i], EntryBuilder);
+    }
----------------
`ArgIt` is incremented in both branches, maybe better to put it at the end of the loop.


Repository:
  rL LLVM

https://reviews.llvm.org/D46018