<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Matthijs,<div><br><div><div>On Aug 11, 2008, at 4:09 AM, Matthijs Kooijman wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><br><blockquote type="cite">I don't have a problem having another class, TargetAddrSpace, to store this <br></blockquote><blockquote type="cite">information. However, I don't think it make sense being a standalone pass. <br></blockquote><blockquote type="cite"> Address spaces seems to part of the TargetData and it seems more natural <br></blockquote><blockquote type="cite">to ask the TargetData to return the TargetAddrSpace object (similar to <br></blockquote><blockquote type="cite">struct layout) to describe the relationships between address spaces.<br></blockquote>This is pretty much what I did in my first patch, but Chris didn't like it.<br>Currently, the TargetData class can be completely described using it's string<br>representation (which is also stored inside a module). Adding address space<br>information to target data breaks this, unless we also make a string<br>representation of the address spaces that we can put in a module.<br><br>I think that having a seperate class is somewhat cleaner, since it also makes<br>sure that this info is only made available to the passes that actually use it.<br><br></div></blockquote><div><br></div><div>If I remember correctly, I was also not fond of passing another TargetAddrSpace reference to the TargetData object. I was hoping that we could encode the information as a target description string like we do for ABI information. I just don't want to end up with too many objects that describe the machine. One can argue that we shouldn't pollute the TargetData since it describes the ABI alignment, size, and object layoutbut I feel that this data fits naturally there. If you and other people feel it is cleaner with a separate pass, I'm fine with it.</div><div><br></div><div>I want to treat my next point with some delicacy as I don't want to start a religious war. I just want to get clarification from the community on the use of multiple inheritance for the case of Phases like AllDisjointAddrspaces. From what I can gather, the use of multiple inheritance is to separate the interface (TargetAddrSpace) to access data from the interface of the phase (ImmutablePhase). In this case, will we ever create a concrete class from TargetAddrSpace that doesn't also derive from ImmutablePass? If not, I don't think is worth using multiple inheritance in this case.</div><div><br></div><blockquote type="cite"><div><blockquote type="cite"><font class="Apple-style-span" color="#144FAE"><font class="Apple-style-span" color="#006312">[Deleted text]</font></font></blockquote><blockquote type="cite"><blockquote type="cite">The last part of this patch is an addition to InstCombine to make use of<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">this information: It removes any bitcasts from a subset to a superset<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">address space. It gets at the address space information by requiring the<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">TargetAddrspaces analysis, which will give it the default implementation in<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">all current tools.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">For the case of a GetElementPointer, we are replacing a bitcast to a <br></blockquote><blockquote type="cite">pointer, getelem with a getelem bitcast. The assumption is the latter <br></blockquote><blockquote type="cite">bitcast will hopefully go away when we iterate through those uses.<br></blockquote>Uh? Is this a comment about what the current code or my patch does, or what it<br>should do? I don't understand what you mean here.<br><br></div></blockquote><div><br></div><div>My comment was more on what I thought the patch did and I wanted to confirm that it will cleanup newly generated bit cast that are created. </div><br><blockquote type="cite"><div><blockquote type="cite"><blockquote type="cite">[Deleted Text]</blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">What I'm suggesting is that Alias Analysis can be a client to where we <br></blockquote><blockquote type="cite">store address space information. In the example you gave, alias analysis <br></blockquote><blockquote type="cite">will examine two memory locations, ask the TargetAddressSpace what the <br></blockquote><blockquote type="cite">relationship is and if it is disjoint, it will return no alias. If the <br></blockquote><blockquote type="cite">address spaces are in subset relationship, the alias analysis returns maybe <br></blockquote><blockquote type="cite">unless it has more information. If a client doesn't tell the compiler the <br></blockquote><blockquote type="cite">correct address space information, the client shouldn't expect correct <br></blockquote><blockquote type="cite">answers from coming out of the compiler.<br></blockquote>True, anyone actually using address space should make sure that this info is<br>correct anyway. So, no need for an unknown default?<br><br></div></blockquote><div><br></div><div>That is my feeling.</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite"><blockquote type="cite">Lastly, I'm still not so sure if InstCombine is the right place for this<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">simplification. This needs some more thought, but currently it is a problem<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">that instcombine does not process BitCastConstantExprs. I might end up<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">writing a seperate pass for just this.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I'm not sure either. At some level, what we want is to propagate the most <br></blockquote><blockquote type="cite">precise address space (or restrict) information to its use.<br></blockquote>Exactly.<br><br><blockquote type="cite">This means that ideally we would want to be able to handle copies of the<br></blockquote><blockquote type="cite">value stored in some temporary and track it all the way through to it use.<br></blockquote><blockquote type="cite">InstCombine will not handle this case, e.g, address space 1 is a subset of 2<br></blockquote><blockquote type="cite"> int<1>* ptr = ...<br></blockquote><blockquote type="cite"> int<2>* ptr2 = ptr1+4<br></blockquote><blockquote type="cite"> *ptr2 = ...<br></blockquote>Won't this code produce a bitcast in the IR, which can be propagated? My<br>current patch doesn't do this, but it should be easy to extend it to also<br>propagate a bitcast past pointer arithmetic.<br><br>Ie, it should change<br><br>%tmp = bitcast i32 addrspace(1)* %ptr1 to i32 addrspace(2)*<br>%ptr2 = add i32 addrspace(2)* %tmp, 4<br>store i32 0 i32 addrspace(2)* %ptr2<br><br>to <br><br>%tmp = add i32 addrspace(1)* %ptr, 4<br>%ptr2 = bitcast %tmp to i32 addrspace(2)<br>store i32 0 i32 addrspace(2)* %ptr2<br><br>and then to <br><br>%ptr2 = add i32 addrspace(1)* %ptr, 4<br>store i32 0 i32 addrspace(1)* %ptr2<br><br>Which shouldn't be too hard?<br><br>Coming to think of it, the above won't even use the add instruction, but will<br>probably use a GEP to do the +4. The current code already propagates bitcasts<br>past GEP instructions. Also, I suspect that our C frontends will already<br>produce the second version of the IR for the C code you give.<br><br>Or am I totally missing the point you are making here?<br><br></div></blockquote><div><br></div><div>No, you got my point even though my example is not a good one. If the address calculation was using a variable, I don't think we can fold it into the GEP and we might lose this information. The point I was trying to make is that the information needs to be propagated through any address calculation when possible.</div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"><br></font></div><div><font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><br></div><div>Cheers,</div><div> -- Mon Ping</div></div></body></html>