<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 21, 2016 at 6:14 PM, Eduard Burtescu via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">eddyb added inline comments.<br>
<span class=""><br>
================<br>
Comment at: include/llvm/Analysis/ValueTracking.h:244<br>
@@ -244,1 +243,3 @@<br>
+  bool isDereferenceablePointer(const Value *V, uint64_t Size,<br>
+                                const DataLayout &DL,<br>
                                 const Instruction *CtxI = nullptr,<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Do we still need to pass the DataLyaout to these functions if they have the size & alignment?<br>
</span>Yes, it's used all over the place in ValueTracking (to get the sizes of other types found during the process). </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: lib/Analysis/ValueTracking.cpp:3260<br>
@@ +3259,3 @@<br>
+    unsigned BaseAlign = DL.getABITypeAlignment(BaseType);<br>
+    if (BaseAlign < Align)<br>
+      BaseAlign = Align;<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Where did this case come from?<br>
</span>Previously, if `Align` was 0, then `BaseAlign` would take over.<br>
Instead, I'm taking the maximum alignment required by either, but I'm not 100% sure it does the exact same thing, only that it passes the tests.<br></blockquote><div><br></div><div>I think it'd be best to preserve the old behavior. Feel free to add a // FIXME: Is this really the right thing, shouldn't we use the blah blah blah<br>or the like.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: lib/Analysis/ValueTracking.cpp:3308<br>
@@ -3307,3 +3302,1 @@<br>
<br>
-  if (Ty->isSized()) {<br>
-    APInt Offset(DL.getTypeStoreSizeInBits(VTy), 0);<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Where'd this check go?<br>
</span>Based on the fact that you can't get the `Size` necessary to call this function if you have an unsized type.<br></blockquote><div><br></div><div>So was this check unnecessary all along? Or are callers doing an equivalent check now?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
But I can add the check back, as `if (Size > 0)`, if you want.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1779<br>
@@ -1778,3 +1778,3 @@<br>
     // isDereferenceablePointer -> deref attribute<br>
-    if (isDereferenceablePointer(DerivedPtr, DL)) {<br>
+    if (isDereferenceablePointer(DerivedPtr, 1, DL)) {<br>
       if (Argument *A = dyn_cast<Argument>(DerivedPtr)) {<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Why is it only '1', rather than 'Bytes' used below?<br>
</span>I recall this being `i8*`, but if that is not the case, I'll use `Bytes` instead.<br></blockquote><div><br></div><div>If it's obviously i8* in the code, that's OK (could you tell me where to look - even if it's a few lines nearby, that'd be helpful), might be worth a comment or assertion.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D16425" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16425</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>