<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Matthijs,<div><br></div><div>Sorry for not responding earlier. I have a few comments.</div><div><br><div><div>On Aug 7, 2008, at 7:41 AM, Matthijs Kooijman wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hi Mon Ping,<br><br>I've again attached a patch, wich lets LLVM know about about the relations<br>between different address spaces. Instead of cramming this info in with<br>TargetData (with all kinds of unwanted side effects, especially for the IR), I<br>opted to create a new pass, TargetAddrspaces, which holds this information.<br>Just like TargetData, this can be added to the passmanager by the tool running<br>the passes.<br><br>Unlike TargetData, however, I wanted to be able to subclass TargetAddrspaces<br>to change the behaviour. To make this possible, I made TargetAddrspaces an<br>analysis group, with a default implementation AllDisjointAddrspaces (which, as<br>the name suggests, returns Disjoint for all combinations). Having a default<br>implementations is useful, so tools are not required to add a TargetAddrspaces<br>pass to a passmanager.<br><br></div></blockquote><div><br></div><div>I don't have a problem having another class, TargetAddrSpace, to store this information. However, I don't think it make sense being a standalone pass. Address spaces seems to part of the TargetData and it seems more natural to ask the TargetData to return the TargetAddrSpace object (similar to struct layout) to describe the relationships between address spaces. BTW, there is a comment in TargetAddrspaces.h that indicate the default is that all address spaces are equivalent. I assume you meant disjoint here.</div><br><blockquote type="cite"><div>The last part of this patch is an addition to InstCombine to make use of this<br>information: It removes any bitcasts from a subset to a superset address<br>space. It gets at the address space information by requiring the<br>TargetAddrspaces analysis, which will give it the default implementation in<br>all current tools.</div></blockquote><div><br></div><div>For the case of a GetElementPointer, we are replacing a bitcast to a pointer, getelem with a getelem bitcast. The assumption is the latter bitcast will hopefully go away when we iterate through those uses.</div><div><div><br></div></div><blockquote type="cite"><div><br><br>So, this requires a minimal amount of changes, the current code won't know the<br>difference, and in our custom frontend we provide another implementation of<br>TargetAddrspaces and things work very well.<br><br>There are a number of issues still, though. I'm not 100% sure that using an<br>Analysis pass for this is correct, but currently only analysis passes can be<br>grouped (though I have the suspicion that this is mainly a question of naming,<br>really).<br><br>Additionally, the "Writing an LLVM Pass" document states: "There must be<br>exactly one default implementation available at all times for an Analysis<br>Group to be used. Only default implementation can derive from ImmutablePass."<br><br>I'm not completely sure why there cannot be other implementations that<br>derive from ImmutablePass. In this case, I want to have any implementation of<br>TargetAddrspaces also derive from ImmutablePass, because it makes sense. In<br>practice, this works as well, our custom implementeation as well as the<br>default implementation derive from ImmutablePass and everything compiles and<br>runs fine. Is this perhaps an outdated statement?<br><br>Mon Ping suggests using address space information for alias analysis as well,<br>which seems to make sense. In effect this is a form of type-based alias<br>analysis, but different address spaces don't preclude pointers from being<br>equal. A problem here is that pointers in disjoint address spaces would be<br>marked as not aliasing, but when the default relation is disjoint this is not<br>so conservative. This might require an extra option "Unknown", which can be<br>used as the default instead of "Disjoint". For Unknown, any pass can do the<br>conservative thing.</div></blockquote><div><br></div><div>What I'm suggesting is that Alias Analysis can be a client to where we store address space information. In the example you gave, alias analysis will examine two memory locations, ask the TargetAddressSpace what the relationship is and if it is disjoint, it will return no alias. If the address spaces are in subset relationship, the alias analysis returns maybe unless it has more information. If a client doesn't tell the compiler the correct address space information, the client shouldn't expect correct answers from coming out of the compiler.</div><div><br></div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"><br></font><br>Lastly, I'm still not so sure if InstCombine is the right place for this<br>simplification. This needs some more thought, but currently it is a problem<br>that instcombine does not process BitCastConstantExprs. I might end up writing<br>a seperate pass for just this.<br><br></div></blockquote><div><br></div><div>I'm not sure either. At some level, what we want is to propagate the most precise address space (or restrict) information to its use. This means that ideally we would want to be able to handle copies of the value stored in some temporary and track it all the way through to it use. InstCombine will not handle this case, e.g, address space 1 is a subset of 2</div><div> int<1>* ptr = ...</div><div> int<2>* ptr2 = ptr1+4</div><div> *ptr2 = ...</div><div><br></div><div>InstCombine will not cleanup this case though a copy propagation phase could clean this up.</div><div><br></div><div>-- Mon Ping</div></div></div></body></html>