690339 - [aot] AOTCompiler needs the GCMember treatment

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Forked off from Bug 690320.

Before we land Bug 685671 to completion, we need to remove the uses of MMgc::WriteBarrier<T*> from AOTCompiler.cpp

From Bug 690320, comment 1:
Another short-term solution is to replace the three uses of WriteBarrierRC<T*> in AOTCompiler.cpp with GCMember<T>; that might be a little trickier than it sounds because the AOTCompiler members aren't really WriteBarrierRC<T*> but rather WriteBarrierRC<T*>*, and now I'm idly wondering whether the level of indirection there is going to screw up the template-fu type reflection that GCMember does.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A: gcmember-ify AOTCompiler

Here's an attempt to get things building after Bug 685871 lands.  (I lack sufficient confidence in this patch to just go ahead and land it to unblock integration; therefore I have backed out 6575:c28201c825db, as described in Bug 690320, comment 1 and logged in Bug 690320, comment 3.)

Alex: I am ignorant of the AOTCompiler.  Where is abcTypes used?  Do I need to actually link in aot-code in some manner?  Should I be looking in the Flash Player to find example uses of abcTypes?  Also, Why does the abcTypes definition have some String/Namespace members that have the WriteBarrierRC wrapper and others that do not?

Tom: I'm including you for feedback since I figured you might find fault with my use of an explicit RCObject::GCMember reference, and wanted to see if you had an alternative suggestion.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 563395 [details] [diff] [review]
patch A: gcmember-ify AOTCompiler

(adding F? for alok since I had him as one of the reviewers back when this patch was attached to Bug 690320; alok, feel free to remove the feedback request if you do not think yourself an appropriate reviewer.)

Comment 3

[Alex Macdonald]

(In reply to Felix S Klock II from comment #1)
> Alex: I am ignorant of the AOTCompiler.  Where is abcTypes used? 

The AOT compiler needs an easy way of finding the type information for all the types it cares about. This struct is essentially just a mapping from some string to a type which the compiler can use to perform a simple kind of reflection on the generated bitcode so it can find the types it wants. If you look in the flash trunk repository you'll see the reference in the AOTCompiler java code.

> Do I need to actually link in aot-code in some manner?  Should I be looking in the
> Flash Player to find example uses of abcTypes?  

You (or someone) will probably need to check that this doesn't break the AOT build in AIR, the only reason that would happen is if this new template widget is structurally different from the old one. 

> Also, Why does the abcTypes definition have some String/Namespace members that have the WriteBarrierRC
> wrapper and others that do not?

The writebarriers I believe we need to know because when we generate code for op_getslot we need to be able to understand the structure of the writebarrier so LLVM can correctly index into it to pull out the real member.

The non-WB variants are just for general type usage everywhere else e.g. function signatures etc

Looks good to me, but it would be worth validating that by making the change in mainline and doing an AIR iOS build

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Alex Macdonald from comment #3)
> Looks good to me, but it would be worth validating that by making the change
> in mainline and doing an AIR iOS build

Will do; I think I'm among the lucky few to actually deploy a self-built AOT app and runtime to an iPod.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 563395 [details] [diff] [review]
patch A: gcmember-ify AOTCompiler

(R+'ing on alexmac's behalf; for some reason his Bugzilla ui doesn't show the flag switching combo box.)

Comment 6

[Tommy Reilly]

Can we get a better explanation for why write barriers are needed here?   Write barrier fields are just pointers as far as LLVM is concerned and I suspect we can just use raw pointers here.

Comment 7

[Alex Macdonald]

(In reply to Tommy Reilly from comment #6)
> Can we get a better explanation for why write barriers are needed here?  
> Write barrier fields are just pointers as far as LLVM is concerned and I
> suspect we can just use raw pointers here.

When we generate code for a getslot (and that slot happens to be a writebarriered pointer) llvm needs to be able to extract the pointer from that field. Even though the writebarrier is "just a pointer" at the bit level LLVM sees it as a structure with one element, we need to generate different code to index into this struct vs just grabbing a raw pointer (see http://llvm.org/docs/GetElementPtr.html)

Comment 8

[Tommy Reilly]

I don't get it.  A write barrier is just a C++ wrapper around a pointer.   To LLVM it's a pointer.  Please tell me how it breaks things if we use a raw pointer here.

Comment 9

[Alex Macdonald]

unless it literally is a pointer at the syntax level (i.e a macro wrapper) then llvm sees foo and bar in the following example as different types :

struct wrapper{
void *ptr;
}

void *foo;
wrapper bar;

And so the code we generate to access foo vs bar.ptr would need to be different.

Comment 10

[Tommy Reilly]

Would a GCRef suffice? I would sign off on that.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Tommy Reilly from comment #10)
> Would a GCRef suffice? I would sign off on that.

Are you asking me, or Alex?

(I would not be able to answer the question comfortably without looking further into what the heck Alex is talking about w.r.t what LLVM is doing.)

Comment 12

[Alok Manchanda]

(In reply to Felix S Klock II from comment #2)

Ugh I can't figure out how to remove the feedback request. Since you already have Alex reviewing this, I don't think you need me to.

Comment 13

[Tommy Reilly]

(In reply to Felix S Klock II from comment #11)
> Are you asking me, or Alex?

Alex, GCMember inherits from GCRef and I would much rather depend on that and use GCRef here than use RCObject::GCMember in some struct that isn't GC memory.

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 563395 [details] [diff] [review]
patch A: gcmember-ify AOTCompiler

Removing Alok (see comment 12).

(I would assume that Alok could also answer Tommy's question to Alex from comment 10 and comment 13.  But if we need to wait for Alex to weigh in, so be it.)

Comment 15

[Alex Macdonald]

Sorry for the late response I was at MAX and then on PTO.

High level answer: yes we can fix fix the struct to either use the new gcmember/gcref syntax / inherit from gcroot and aot will continue to work. It might require some small tweak to the aot compiler or not, I can't be sure without further investigation.

But I'm no longer on the AOT team so I'm not the right person to sign off on this, and I've not been looking at the code for quite some time so I'm slightly hazy about the exact details. All I know is that when constructing llvm bitcode the aot compiler needs to have type information for various things used in the VM, if those types are incorrect then it will no doubt start generating bogus code.

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Alex Macdonald from comment #15)
> High level answer: yes we can fix fix the struct to either use the new
> gcmember/gcref syntax / inherit from gcroot and aot will continue to work.
> It might require some small tweak to the aot compiler or not, I can't be
> sure without further investigation.
> 
> But I'm no longer on the AOT team so I'm not the right person to sign off on
> this [...]

Was I wrong to remove Alok from the feedback list then?

Or can you point me to another person to pull into the discussion here?

Comment 17

[Alex Macdonald]

Yeah, Alok/Damanjit are the right people to sign off on it

Comment 18

[Alok Manchanda]

Sorry I stopped following this bug the last couple of weeks. I'll take a look at whether we can use GCRef here.