<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 4 March 2014 13:59, Evan Cheng <span dir="ltr"><<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word">Sorry, I need to have better mail filtering.<div><br></div><div>PseudoSourceValue is a bad hack that's long on my kill list. Thank you for tackling this. Would you consider going further than your current patch and kill it completely?</div>
</div></blockquote><div><br></div><div>I'm trying to get some undefined behaviour removed from LLVM, and I'm pulling the thread of llvm::Value having a vtable. This would be a diversion ...<br></div><div><br></div>
<div>... but I realize that doing this patch now and then removing PSV later would break out of tree targets twice, and it would be nice to avoid that. That's a good reason to do it all at once.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>The idea is to enhance MachinePointerInfo so the value can be an IR pointer value (i.e. const Value *V) or some immediate that represents special type such as constant pool, stack objects, etc. As a bonus, we would also want a mechanism to represent target specific tag to model special dependencies (common in non-CPU world). Your change in MachineMemOperand.h seems half way there.<br>
</div></div></blockquote><div><br></div><div>Could you expand on this? How would that immediate be different from PseudoSourceValue after this change? Would we have the four PSVs (stack, GOT, JumpTable, ConstantPool) plus a Value PSV that contains a Value*? And targets could add their own to the list of 4 I mentioned?</div>
<div><br></div><div>Basically, I don't know enough to fill in the details of design you're describing, but I'm willing to do the actual coding/testing/debugging bits.</div><div><br></div><div>Nick</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div></div><div><span class=""><font color="#888888"><div>
<br></div><div>Evan</div></font></span><div><div class="h5"><div><br><div><div>On Mar 4, 2014, at 1:37 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:</div><br>
<blockquote type="cite"><div style="font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
On Mon, Mar 3, 2014 at 6:09 PM, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:<br><blockquote type="cite">1 week ping!<br><br>+Eric Christopher, I'd also appreciate a review.<br>
</blockquote><br>Sure. It's a pretty large patch, I noticed a few things and really<br>hope we have testcases for a lot of it :)<br><br>+ bool isFixed;<br><br>Appears to be completely undocumented :)<br><br><br>+ template<typename T, typename U><br>
+ struct DenseMapInfo<PointerUnion<T, U> > {<br><br>As does this :)<br><br>+ DstInfo, MachinePointerInfo((Value*)0));<br><br>MachinePointerInfo() ?<br><br>(and in the other places that have the same idiom)<br>
<br>// Eventually these should be uniqued on LLVMContext rather than in a managed<br>// static. For now, we can safely use the global context for the time being to<br>// squeak by.<br>-PseudoSourceValue::PseudoSourceValue(enum ValueTy Subclass) :<br>
- Value(Type::getInt8PtrTy(getGlobalContext()),<br>- Subclass) {}<br>+PseudoSourceValue::PseudoSourceValue(bool isFixed) : isFixed(isFixed) {}<br><br>Comment appears out of date now.<br><br>Otherwise looks ok to me.<br>
<br>-eric<br><br><blockquote type="cite"><br>Hal, do you have any further review comments? (Does it look good to you, but<br>you don't feel like you have sufficient knowledge to OK a commit?)<br><br>I think that between the three of us, Hal, Eric and myself, that would be<br>
sufficient eyes to approve the patch for commit.<br><br>As a note to everybody else, this is likely to break out of tree targets<br>when it lands. My current plan is to send an email to llvm-dev shortly<br>before committing this patch, after I get the OK to commit.<br>
<br>Nick<br><br><br>On 24 February 2014 17:19, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:<br><blockquote type="cite"><br>+cc Evan Cheng<br><br>Evan, I'm not sure whether you're still the right person to review this. I<br>
consider it a substantive change to how the backend works, even though the<br>line count of the patch is small. If nothing else, I figure you want to be<br>aware of this patch before it lands.<br><br>Nick<br><br><br>On 24 February 2014 17:01, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:<br>
<blockquote type="cite"><br>On 24 February 2014 16:45, Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>> wrote:<br><blockquote type="cite"><br>----- Original Message -----<br><blockquote type="cite">
From: "Nick Lewycky" <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>><br>To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
Cc: "Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>><br>Sent: Monday, February 24, 2014 6:31:34 PM<br>Subject: Re: patch: remove PseudoSourceValue from Value.<br>
<br><br><br><br>On 24 February 2014 16:20, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> > wrote:<br><br><br>What is the underlying motivation for doing this?<br><br><br><br>The proximate cause is that I'm trying to remove the vtable from<br>
llvm::Value, and in order to do that I'm going to write dispatch<br>functions that switch on the ValueID and call the appropriate<br>function from the most-derived-type. If I do that, then we have<br>lib/IR placing calls to lib/CodeGen, which is a layering violation.<br>
<br><br>At a higher level, PSV does not belong there. It is not part of the<br>IR. No IR passes use or understand it. It's not documented in the<br>LangRef. Frankly, it's a hack -- and it's a great hack that and I<br>
understand its utility for lib/CodeGen -- but that doesn't change<br>the fact that it's not part of llvm's IR. I've just never been<br>motivated to do something about it until now.<br></blockquote><br>Thanks for explaining!<br>
<br> AliasAnalysis::AliasResult AAResult = AA->alias(<br>- AliasAnalysis::Location(MMOa->getValue(), Overlapa,<br>- UseTBAA ? MMOa->getTBAAInfo() : 0),<br>- AliasAnalysis::Location(MMOb->getValue(), Overlapb,<br>
- UseTBAA ? MMOb->getTBAAInfo() : 0));<br>+ AliasAnalysis::Location(MMOa->getValue(), Overlapa,<br>+ UseTBAA ? MMOa->getTBAAInfo() : 0),<br>+ AliasAnalysis::Location(MMOb->getValue(), Overlapb,<br>
+ UseTBAA ? MMOb->getTBAAInfo() : 0));<br><br>This is purely an indentation change, can you please commit this<br>separately?<br></blockquote><br><br>Done.<br><br><blockquote type="cite"><br>
- MapVector<const Value *, std::vector<SUnit *> >::iterator IE =<br>+ MapVector<PointerUnion<const Value *, const PseudoSourceValue<br>*>,<br>+ std::vector<SUnit *> >::iterator IE =<br>
<br>These are starting to get really ugly. Can you please do something so<br>that we don't have to use these really long local types in many places (like<br>make a local typedef)?<br></blockquote><br><br>Done. Updated patch attached.<br>
<br>Nick<br><br><br></blockquote><br></blockquote><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br><br></blockquote>_______________________________________________<br>llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div>
</blockquote></div><br></div></div></div></div></div></blockquote></div><br></div></div>