[PATCH] D29105: Fix regalloc assignment of overlapping registers

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked 3 inline comments as done.
rampitec added inline comments.


================
Comment at: lib/CodeGen/SplitKit.cpp:535
+          if (TRI.getSubRegIndexLaneMask(I) == LM) {
+            SubIdx = I;
+            break;
----------------
MatzeB wrote:
> MatzeB wrote:
> > rampitec wrote:
> > > alex-t wrote:
> > > > alex-t wrote:
> > > > > What if in parent LiveInterval there are more then one subrange (i.e. several not adjacent lanes are in use) ?  Let's say we copy sub0 and sub2.
> > > > > LaneMask will be 1010 and the condition "TRI.getSubRegIndexLaneMask(I) == LM" will never met.
> > > > > Given the assert below this, why this never expected to happen?
> > > > Oops... forget it )  I missed that getSubRegIndexLaneMask and setSubreg treat the parameter as a plane integer...
> > > It looks like it does not happen, but in case if lane combination is not supported by target here is the assert. To me it is better to see an assert rather than silently clobber register.
> > This can definitely happen. And we probably need a better scheme to deal with that. At the very least you should make it a report_fatal_error() so even release compilers abort instead of producing invalid code!
> Doesn't this also fail if a full register is copied (the parts that do not have all lanes alive could be somewhere else in the program).
If all lanes are used that is recorded in the liveness, then the full register is copied. That is correct. If some lanes are unused, then we are looking for a subreg to cover only used lanes, and if found that is correct again. The only problem is as discussed if such a subreg cannot be found. Too bad, I cannot insert more than copy here, neither extend liveness here to cover all lanes if we fail to find a subreg index.

I.e. from correctness point of view it seems to be fine, with exception of subreg index absence of course. I will insert fatal error here.


================
Comment at: lib/CodeGen/SplitKit.cpp:535
+          if (TRI.getSubRegIndexLaneMask(I) == LM) {
+            SubIdx = I;
+            break;
----------------
rampitec wrote:
> MatzeB wrote:
> > MatzeB wrote:
> > > rampitec wrote:
> > > > alex-t wrote:
> > > > > alex-t wrote:
> > > > > > What if in parent LiveInterval there are more then one subrange (i.e. several not adjacent lanes are in use) ?  Let's say we copy sub0 and sub2.
> > > > > > LaneMask will be 1010 and the condition "TRI.getSubRegIndexLaneMask(I) == LM" will never met.
> > > > > > Given the assert below this, why this never expected to happen?
> > > > > Oops... forget it )  I missed that getSubRegIndexLaneMask and setSubreg treat the parameter as a plane integer...
> > > > It looks like it does not happen, but in case if lane combination is not supported by target here is the assert. To me it is better to see an assert rather than silently clobber register.
> > > This can definitely happen. And we probably need a better scheme to deal with that. At the very least you should make it a report_fatal_error() so even release compilers abort instead of producing invalid code!
> > Doesn't this also fail if a full register is copied (the parts that do not have all lanes alive could be somewhere else in the program).
> If all lanes are used that is recorded in the liveness, then the full register is copied. That is correct. If some lanes are unused, then we are looking for a subreg to cover only used lanes, and if found that is correct again. The only problem is as discussed if such a subreg cannot be found. Too bad, I cannot insert more than copy here, neither extend liveness here to cover all lanes if we fail to find a subreg index.
> 
> I.e. from correctness point of view it seems to be fine, with exception of subreg index absence of course. I will insert fatal error here.
Agree, will insert fatal error and update the review.


Repository:
  rL LLVM

https://reviews.llvm.org/D29105