[webkit-reviews] review denied: [Bug 200181] Identify memcpy loops in b3 : [Attachment 376570] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 200181: Identify memcpy loops in b3
https://bugs.webkit.org/show_bug.cgi?id=200181

Attachment 376570: Patch

https://bugs.webkit.org/attachment.cgi?id=376570&action=review




--- Comment #53 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 376570
  --> https://bugs.webkit.org/attachment.cgi?id=376570
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376570&action=review

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:147
> +void __attribute__ ((optnone)) fastForwardCopy32(uint32_t* dst, const
uint32_t* src, size_t size)
> +{
> +    for (size_t i = 0; i < size; ++i)
> +	   dst[i] = src[i];
> +}

why would this ever be reached? I think we should skip this until we enable
this on non x86

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:176
> +    void reduceByteCopyLoopsToMemcpy()

can we file some bugs on enhancing this phase?

- to Wsam
- More than elemSize == 4
- Other loop kinds

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:195
> +	   auto extractArrayInfo = [&] (Value* value) ->
std::unique_ptr<AddrInfo> {

nit: why unique_ptr? Why not just optional? That way it'll just be stack
allocated by LLVM

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:254
> +	   auto loop = m_proc.naturalLoops().innerMostLoopOf(m_block);

you're not doing a null check here. Please add a test. This should be an insta
crash.

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:263
> +	       for (unsigned i = 0; i < loop->size(); ++i)
> +		   loopInnerBlocks.add(loop->at(i));

this should contain the header, right?

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:266
> +	       if (!loopInnerBlocks.contains(load->owner))
> +		   return;

can you assert that it contains store->owner just for completeness?

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:334
> +		   if (m_proc.dominators().dominates(up->owner, loopHead))
> +		       startingIndex = up;

don't we want strictly dominates here? Could be worth a test too. I believe if
up->owner == loopHead, we'll just always store at index 0?

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:354
> +	   if (!startingIndex->isInt(0))
> +	       return;

style nit: seems like a weird place to put this line given you check
updateIndex up above

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:447
> +	       loopBound);

where do we ever ensure that loopBound can be hoisted from the loop? Don't we
need to do that? What if loopBound is defined in the loop? Seems wrong to do
this in such a scenario. Seems like it'd be easy to add a test where loopBound
is a constant defined in the loop and you generate invalid SSA here.

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:482
> +#if CPU(X86_64)

can we file a bug to enable this on arm64?