<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 4 March 2014 13:37, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">On Mon, Mar 3, 2014 at 6:09 PM, Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>> wrote:<br>
> 1 week ping!<br>
><br>
> +Eric Christopher, I'd also appreciate a review.<br>
<br>
</div>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></blockquote><div><br></div><div>Thanks! Updated patch attached with all these changes.</div><div><br></div><div>Nick</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><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">nlewycky@google.com</a>> wrote:<br>
>><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">nlewycky@google.com</a>> wrote:<br>
>>><br>
>>> On 24 February 2014 16:45, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br>
>>>><br>
>>>> ----- Original Message -----<br>
>>>> > From: "Nick Lewycky" <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>><br>
>>>> > To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
>>>> > Cc: "Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu">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">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>
>>>><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>
>>><br>
>>><br>
>>> Done.<br>
>>><br>
>>>><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>
>>><br>
>>><br>
>>> Done. Updated patch attached.<br>
>>><br>
>>> Nick<br>
>>><br>
>>><br>
>><br>
><br>
><br>
</div></div><div class="HOEnZb"><div class="h5">> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">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>
</div></div></blockquote></div><br></div></div>