This validation rule is broken. It should always be valid to take: Foo(@x, @x) and turn it into: a: ValueRep(@x) b: ValueRep(@x) Foo(@a, @b) or: y: Identity(@x) Foo(@x, @y) That's because it should be possible to rewire any data flow edge something that produces an equivalent value. This validation rule means that such rewirings are invalid on GetByOffset/PutByOffset.
This is what the IR actually looks like when we crash: 2641:< 1:-> ValueRep(Check:DoubleRep:@858<Double>, JS|PureInt, Bytecodedouble, bc#9) 889:<!0:-> CheckStructure(Check:Cell:@2641, MustGen, [%Ea:Object], R:JSCell_structureID, Exits, bc#9) 2642:< 1:-> ValueRep(Check:DoubleRep:@858<Double>, JS|PureInt, Bytecodedouble, bc#9) 2643:< 1:-> ValueRep(Check:DoubleRep:@858<Double>, JS|PureInt, Bytecodedouble, bc#9) 890:< 1:-> GetByOffset(Check:KnownCell:@2642, Check:KnownCell:@2643, JS|PureInt|UseAsInt, Nonboolint32, id24{Ca}, 0, inferredType = Int32, R:NamedProperties(24), Exits, bc#9) predicting Nonboolint32 We fail validation at the GetByOffset. But the GetByOffset is dead anyway, and even if it wasn't, it would be OK to ignore the first child of the GetByOffset when doing analysis even if it was different from the second child. Both children produce the same value!
Created attachment 283263 [details] the patch
Comment on attachment 283263 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=283263&action=review > Source/JavaScriptCore/ChangeLog:9 > + This removes an incorrect validation rule and replaces it with a FIXME about how to make this > + aspect of IR easier to validate soundly. You should say why the validation rule is not valid. It's not really clear what this patch is actually fixing without that information.
(In reply to comment #3) > Comment on attachment 283263 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283263&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + This removes an incorrect validation rule and replaces it with a FIXME about how to make this > > + aspect of IR easier to validate soundly. > > You should say why the validation rule is not valid. It's not really clear > what this patch is actually fixing without that information. I'm assuming you just want me to copy-paste the bug description into the ChangeLog.
(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 283263 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=283263&action=review > > > > > Source/JavaScriptCore/ChangeLog:9 > > > + This removes an incorrect validation rule and replaces it with a FIXME about how to make this > > > + aspect of IR easier to validate soundly. > > > > You should say why the validation rule is not valid. It's not really clear > > what this patch is actually fixing without that information. > > I'm assuming you just want me to copy-paste the bug description into the > ChangeLog. Yeah, that would be fine.
Created attachment 283268 [details] the patch
Comment on attachment 283263 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=283263&action=review r=me if you add the description. >>>> Source/JavaScriptCore/ChangeLog:9 >>>> + aspect of IR easier to validate soundly. >>> >>> You should say why the validation rule is not valid. It's not really clear what this patch is actually fixing without that information. >> >> I'm assuming you just want me to copy-paste the bug description into the ChangeLog. > > Yeah, that would be fine. I actually missed that there was a description. If you do that I think the patch is g2g.
Comment on attachment 283268 [details] the patch r=me
Comment on attachment 283268 [details] the patch Clearing flags on attachment: 283268 Committed r203034: <http://trac.webkit.org/changeset/203034>
All reviewed patches have been landed. Closing bug.