[JSC] FTLB3Output generates some invalid ZExt32

[Benjamin Poulain]

[JSC] FTLB3Output generates some invalid ZExt32

[Benjamin Poulain]

Created attachment 266702 [details]
Patch

[Filip Pizlo]

Comment on attachment 266702 [details]
Patch

Can you pick a consistent naming convention? I like zeroExtFooToBar. The zeroExtPtr and zeroExtTo32 feel like they should be renamed. 

I was previously hoping that zeroExt could continue to take a type as an argument and the B3 output could be smart enough to avoid emitting it when the requested type matches the input type. That feels like the better abstraction since we already know the type of the input and having one method is probably better than having three.

[Benjamin Poulain]

The reason for splitting into 3 methods is to make the cleanup easy later.
Find any use, delete the call.

[Filip Pizlo]

(In reply to comment #3)
> The reason for splitting into 3 methods is to make the cleanup easy later.
> Find any use, delete the call.

But this puts us in an inconsistent state.  The current approach to this problem in other casts, like castToInt32, is:

    LValue castToInt32(LValue value)
    {
        return value->type() == B3::Int32 ? value :
            m_block->appendNew<B3::Value>(m_proc, B3::Trunc, origin(), value);
    }

Why wouldn't that work here?  You could just say:

    LValue zeroExt(LValue value, LType type)
    {
        return type == value->type() ? value : m_block->appendNew<B3::Value>(m_proc, B3::ZExt32, type, origin(), value);
    }

If we don't want to do that for zeroExt(), then I think that your patch should also change how castToInt32() works, so that we are consistent.

But I think that it makes sense for FTL::Output to abstract casts using the existing zeroExt(Value, Type) API.  I think that this one API covers all of the things that your new functions do, and we could leave it this way forever.  Even if we get rid of LLVM, a zeroExt(Value, Type) operation still makes sense.

[Benjamin Poulain]

Created attachment 267687 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267687 [details]
Patch

Clearing flags on attachment: 267687

Committed r194315: <http://trac.webkit.org/changeset/194315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.