Bug 153065

Created attachment 268870 [details] Patch

Attachment 268870 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/testb3.cpp:154: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:169: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 268870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268870&action=review r=me > Source/JavaScriptCore/ChangeLog:9 > + There are two available addressing: signed 9bits and unsigne scaled 12bits. typo: "unsigne" ==> "unsigned". > Source/JavaScriptCore/b3/B3LegalizeMemoryOffsets.h:36 > +// compute it explicitely. typo: "explicitely" ==> "explicitly". > Source/JavaScriptCore/b3/testb3.cpp:171 > +void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32) > +{ > + { > + Procedure proc; > + BasicBlock* root = proc.addBlock(); > + int64_t x = -42; > + Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0); > + root->appendNew<ControlValue>( > + proc, Return, Origin(), > + root->appendNew<MemoryValue>( > + proc, Load, Int64, Origin(), > + base, > + offset64)); > + > + char* address = reinterpret_cast<char*>(&x) - offset64; > + CHECK(compileAndRun<int64_t>(proc, address) == -42); > + } > + { > + Procedure proc; > + BasicBlock* root = proc.addBlock(); > + int32_t x = -42; > + Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0); > + root->appendNew<ControlValue>( > + proc, Return, Origin(), > + root->appendNew<MemoryValue>( > + proc, Load, Int32, Origin(), > + base, > + offset32)); > + > + char* address = reinterpret_cast<char*>(&x) - offset32; > + CHECK(compileAndRun<int32_t>(proc, address) == -42); > + } > +} nit: the 64-bit and 32-bit sections are almost identical except for some type and variable differences. Would it be better to templatize the common form and express these 2 sections as 2 instantiations of the template? Something like: template <typename T, B3::Type b3Type> void testLoadWithOffsetImpl(int32_t offset) { Procedure proc; BasicBlock* root = proc.addBlock(); T x = -42; Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0); root->appendNew<ControlValue>( proc, Return, Origin(), root->appendNew<MemoryValue>( proc, Load, b3Type, Origin(), base, offset)); char* address = reinterpret_cast<char*>(&x) - offset; CHECK(compileAndRun<T>(proc, address) == -42); } void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32) { testLoadWithOffsetImpl<int64_t, Int64>(offset64); testLoadWithOffsetImpl<int32_t, Int32>(offset32); }

Comment on attachment 268870 [details] Patch Nice!

R=me too

(In reply to comment #3) > Comment on attachment 268870 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268870&action=review > > r=me > > > Source/JavaScriptCore/ChangeLog:9 > > + There are two available addressing: signed 9bits and unsigne scaled 12bits. > > typo: "unsigne" ==> "unsigned". > > > Source/JavaScriptCore/b3/B3LegalizeMemoryOffsets.h:36 > > +// compute it explicitely. > > typo: "explicitely" ==> "explicitly". > > > Source/JavaScriptCore/b3/testb3.cpp:171 > > +void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32) > > +{ > > + { > > + Procedure proc; > > + BasicBlock* root = proc.addBlock(); > > + int64_t x = -42; > > + Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0); > > + root->appendNew<ControlValue>( > > + proc, Return, Origin(), > > + root->appendNew<MemoryValue>( > > + proc, Load, Int64, Origin(), > > + base, > > + offset64)); > > + > > + char* address = reinterpret_cast<char*>(&x) - offset64; > > + CHECK(compileAndRun<int64_t>(proc, address) == -42); > > + } > > + { > > + Procedure proc; > > + BasicBlock* root = proc.addBlock(); > > + int32_t x = -42; > > + Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0); > > + root->appendNew<ControlValue>( > > + proc, Return, Origin(), > > + root->appendNew<MemoryValue>( > > + proc, Load, Int32, Origin(), > > + base, > > + offset32)); > > + > > + char* address = reinterpret_cast<char*>(&x) - offset32; > > + CHECK(compileAndRun<int32_t>(proc, address) == -42); > > + } > > +} > > nit: the 64-bit and 32-bit sections are almost identical except for some > type and variable differences. Would it be better to templatize the common > form and express these 2 sections as 2 instantiations of the template? > Something like: > > template <typename T, B3::Type b3Type> > void testLoadWithOffsetImpl(int32_t offset) > { > Procedure proc; > BasicBlock* root = proc.addBlock(); > T x = -42; > Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), > GPRInfo::argumentGPR0); > root->appendNew<ControlValue>( > proc, Return, Origin(), > root->appendNew<MemoryValue>( > proc, Load, b3Type, Origin(), > base, > offset)); > > char* address = reinterpret_cast<char*>(&x) - offset; > CHECK(compileAndRun<T>(proc, address) == -42); > } > > void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32) > { > testLoadWithOffsetImpl<int64_t, Int64>(offset64); > testLoadWithOffsetImpl<int32_t, Int32>(offset32); > } For what it's worth we haven't been trying too hard to reduce code duplication in the tests. Reducing code duplication takes work, and it is most profitable when you intend to somehow evolve the code later - so you don't have to change the code in more than one place. But we don't really change test code after its written. Even if we made a B3 change that somehow made a test not relevant, we would probably write a new test and keep the old one for regression coverage. It's conceivable that we would change IR in a way that makes tests break or not compile, but then we would probably have to search-replace all of testb3 anyway. TL:DR; I'm not opposed to deduplicating code but it's not a priority for tests.

Created attachment 268875 [details] Patch for landing

Comment on attachment 268875 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=268875&action=review > Source/JavaScriptCore/dfg/DFGCommon.h:41 > -#define FTL_USES_B3 0 > +#define FTL_USES_B3 1 Is this intentional?

(In reply to comment #8) > Comment on attachment 268875 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268875&action=review > > > Source/JavaScriptCore/dfg/DFGCommon.h:41 > > -#define FTL_USES_B3 0 > > +#define FTL_USES_B3 1 > > Is this intentional? Shit! Good catch. I forgot the -g.

Created attachment 268876 [details] Patch for landing

I kept the duplicated code. I find it easier to debug on device since the only thing we get at first is the faulty line number.

Comment on attachment 268876 [details] Patch for landing Clearing flags on attachment: 268876 Committed r194969: <http://trac.webkit.org/changeset/194969>

All reviewed patches have been landed. Closing bug.

This change appears to have broken the iOS build: <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/builds/1947> Undefined symbols for architecture arm64: "__ZN3JSC2B35Value2asINS0_11MemoryValueEEEPT_v", referenced from: __ZN3JSC2B321legalizeMemoryOffsetsERNS0_9ProcedureE in B3LegalizeMemoryOffsets.o

(In reply to comment #14) > This change appears to have broken the iOS build: > <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/ > builds/1947> > > Undefined symbols for architecture arm64: > "__ZN3JSC2B35Value2asINS0_11MemoryValueEEEPT_v", referenced from: > __ZN3JSC2B321legalizeMemoryOffsetsERNS0_9ProcedureE in > B3LegalizeMemoryOffsets.o Same here :(

Re-opened since this is blocked by bug 153075

Created attachment 269111 [details] Patch for landing

Comment on attachment 269111 [details] Patch for landing Clearing flags on attachment: 269111 Committed r195159: <http://trac.webkit.org/changeset/195159>

All reviewed patches have been landed. Closing bug.