Created attachment 265257 [details] Patch

Comment on attachment 265257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265257&action=review > Source/JavaScriptCore/b3/air/AirInstInlines.h:63 > - arg = Arg::stack(stackSlot); > + arg = Arg::stack(stackSlot, arg.offset()); Oops. > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:51 > + bool isMove = inst.opcode == Move || inst.opcode == Move32; We shouldn't coalesce Move32's, since they zero-extend.

Committed r192292: <http://trac.webkit.org/changeset/192292>

Comment on attachment 265257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265257&action=review > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:323 > + if (!degree) > + continue; Oh man! This finally turned into a bug, I think! A spill tmp from the previous iteration may have no degree: imagine a tmp that is live everywhere and interferes with everyone, but has one use like: Move %ourTmp, %someOtherTmp Where there are no other tmps live. After spill conversion, this may look like: Move (ourSpill), %newTmp Move %newTmp, %someOtherTmp Of course, we'd rather not get this kind of spill code but it's totally possible because we now have a bunch of random conditions under which we won't slap the spill address directly into the Move. After this happens, assuming that the only thing live was %someOtherTmp, we will have zero degree for %newTmp because the Move is coalescable and does not contribute to interference. Then, we might coalesce %someOtherTmp with %newTmp. Once this happens, if we make the %newTmp be the master, we're in deep trouble because %newTmp is not on any worklist due to these two lines of code! Do you agree that these two lines can be removed?

(In reply to comment #4) > Comment on attachment 265257 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265257&action=review > > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:323 > > + if (!degree) > > + continue; > > Oh man! This finally turned into a bug, I think! A spill tmp from the > previous iteration may have no degree: imagine a tmp that is live everywhere > and interferes with everyone, but has one use like: > > Move %ourTmp, %someOtherTmp > > Where there are no other tmps live. After spill conversion, this may look > like: > > Move (ourSpill), %newTmp > Move %newTmp, %someOtherTmp > > Of course, we'd rather not get this kind of spill code but it's totally > possible because we now have a bunch of random conditions under which we > won't slap the spill address directly into the Move. > > After this happens, assuming that the only thing live was %someOtherTmp, we > will have zero degree for %newTmp because the Move is coalescable and does > not contribute to interference. > > Then, we might coalesce %someOtherTmp with %newTmp. Once this happens, if > we make the %newTmp be the master, we're in deep trouble because %newTmp is > not on any worklist due to these two lines of code! > > Do you agree that these two lines can be removed? https://bugs.webkit.org/show_bug.cgi?id=163509