[PATCH] D30438: SplitKit: Correctly implement partial copies

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: lib/CodeGen/SplitKit.cpp:572
+
+      if (BestIdx == 0)
+        report_fatal_error("Impossible to implement partial COPY");
----------------
MatzeB wrote:
> kparzysz wrote:
> > MatzeB wrote:
> > > kparzysz wrote:
> > > > Would it make sense to add `|| BestCover <= 0`?
> > > `BestCover <= 0` can happen and is fine (it just means the best we could find is an index that covers more unnecessary lanes that we already covered anyway, than lanes we haven't covered yet).
> > Well, wasn't that the original problem, i.e. that we copied too much (which then ended up clobbering lanes marked as unused)?
> No, the PossibleIndexes vector only contains indexes that cover bits that we are allowed to cover. The BestCover is solely about minimizing the amount of lanes we copy twice. We will not copy lanes that we are not supposed to copy.
Ok, I see it now.


================
Comment at: lib/CodeGen/SplitKit.cpp:597
+    if (BestIdx == 0)
+      report_fatal_error("Impossible to implement partial COPY");
+
----------------
This doesn't need to be addressed in this patch, but on a general note---this would be a case where the compiler corners itself into such a situation.  It's not a symptom of simply a bug, but rather a design that can end up in a dead end.  Ideally, we should avoid getting here early enough to have other options.


================
Comment at: test/CodeGen/AMDGPU/splitkit.mir:35
+# in between for the two subregisters that are alive.
+# CHECK-LABEL: name: func1
+name: func1
----------------
This is the only CHECK like in this function.  Did it use to crash?


Repository:
  rL LLVM

https://reviews.llvm.org/D30438