Bug 151905 - [JSC] FTLB3Output generates some invalid ZExt32
Summary: [JSC] FTLB3Output generates some invalid ZExt32
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
Depends on:
Reported: 2015-12-04 20:45 PST by Benjamin Poulain
Modified: 2015-12-19 11:02 PST (History)
6 users (show)

See Also:

Patch (9.23 KB, patch)
2015-12-04 21:09 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2015-12-19 08:32 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2015-12-04 20:45:39 PST
[JSC] FTLB3Output generates some invalid ZExt32
Comment 1 Benjamin Poulain 2015-12-04 21:09:25 PST
Created attachment 266702 [details]
Comment 2 Filip Pizlo 2015-12-04 21:46:08 PST
Comment on attachment 266702 [details]

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.
Comment 3 Benjamin Poulain 2015-12-04 21:49:10 PST
The reason for splitting into 3 methods is to make the cleanup easy later.
Find any use, delete the call.
Comment 4 Filip Pizlo 2015-12-04 22:22:57 PST
(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.
Comment 5 Benjamin Poulain 2015-12-19 08:32:46 PST
Created attachment 267687 [details]
Comment 6 WebKit Commit Bot 2015-12-19 11:02:17 PST
Comment on attachment 267687 [details]

Clearing flags on attachment: 267687

Committed r194315: <http://trac.webkit.org/changeset/194315>
Comment 7 WebKit Commit Bot 2015-12-19 11:02:23 PST
All reviewed patches have been landed.  Closing bug.