<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Dec 18, 2020 at 10:06 AM digger lin <<a href="mailto:digger.llvm@gmail.com">digger.llvm@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">added a new NFC patch <a href="https://reviews.llvm.org/rGd551e40f1cff7a63218f34112bd0dddaa2b12dbb" target="_blank">https://reviews.llvm.org/rGd551e40f1cff7a63218f34112bd0dddaa2b12dbb</a> to address the comment. <br></div></blockquote><div><br>Thanks!<br><br>Though looking at it, the original code seems to have been buggy, rather than just redundant, right? (the condition would never have fired) - the code under the "if" condition hasn't been tested? (since if it had, it would've shown the code to be unreachable) - Could you/someone add the missing test coverage here?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Dec 17, 2020 at 9:17 PM Hubert Tong <<a href="mailto:hubert.reinterpretcast@gmail.com" target="_blank">hubert.reinterpretcast@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 16, 2020 at 8:47 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Dec 15, 2020 at 9:24 AM Hubert Tong <<a href="mailto:hubert.reinterpretcast@gmail.com" target="_blank">hubert.reinterpretcast@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 14, 2020 at 8:36 PM David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Any idea if this is the most suitable fix? Is "Bits" representing negative values? If not, perhaps it'd be more appropriate to remove the >= 0 comparison, rather than to change the type?</div></blockquote><div>For context, this aspect of the implementation will likely be replaced as further functionality (to implement the encoding form when vector parameters are present) is implemented. In terms of NFC evolution of this code, the reparsing loop is not ideal. If the code was not being changed anyway, it would make sense to store the "next position" alongside the encoded bitstring.<br></div><div><br></div><div>As for the signedness of the type: I believe changing the type is appropriate in this case even if the comparison is modified to avoid the subtraction.</div></div></div></blockquote><div><br></div><div>Appropriate in that you think it's a more suitable type for the value being stored in it, even if it weren't for the warning?</div></div></div></blockquote><div>There's been quite the thread that has not gotten consensus one way or the other about preferring `int` (see related: <a href="https://reviews.llvm.org/D63049" target="_blank">https://reviews.llvm.org/D63049</a>). The variable here (aside from representing something that is never negative) has no particular reason to be `unsigned`. So, using `int` to avoid the wraparound at `0` has its advantages. Perhaps I would give the rationale of using `unsigned` more weight if this was part of a function interface.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> I do agree that changing the comparison should also be done; however, the change to the type was the easiest change to eyeball in the context of responding to the breakage.<br></div></div></div></blockquote><div><br>Would be good to clean that up, then - could someone make that change (& mention the commit hash here)? (otherwise it'll be hard to track down the longer the code evolves - without the warning to point to it)<br></div></div></div></blockquote><div><a class="gmail_plusreply" id="gmail-m_7396810060455797406gmail-m_534489265295766740gmail-m_2947951042613827066plusReplyChip-0" href="mailto:digger.llvm@gmail.com" target="_blank">@digger lin</a>, can you post this change or (if the code is ready) the change that removes the current implementation (in order to add functionality)?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 14, 2020 at 8:09 AM via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Author: diggerlin<br>
Date: 2020-12-14T11:08:40-05:00<br>
New Revision: 15f2d4f198380762e9fcf6b456d405078b87ae7a<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a.diff</a><br>
<br>
LOG: [AIX] Fixed "comparison of unsigned expression >= 0 is always true" gcc warnings.<br>
<br>
Summary:<br>
<br>
fixed a Fixed "comparison of unsigned expression >= 0 is always true" gcc warnings.<br>
<a href="http://lab.llvm.org:8011/#/builders/5/builds/2407/steps/2/logs/stdio" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/#/builders/5/builds/2407/steps/2/logs/stdio</a><br>
<br>
the error caused by patch <a href="https://reviews.llvm.org/D92398" rel="noreferrer" target="_blank">https://reviews.llvm.org/D92398</a><br>
<br>
Added: <br>
<br>
<br>
Modified: <br>
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp b/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp<br>
index 02a425044c75..d364eb9d3996 100644<br>
--- a/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp<br>
+++ b/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp<br>
@@ -67,7 +67,7 @@ bool PPCFunctionInfo::isLiveInZExt(Register VReg) const {<br>
<br>
void PPCFunctionInfo::appendParameterType(ParamType Type) {<br>
uint32_t CopyParamType = ParameterType;<br>
- unsigned Bits = 0;<br>
+ int Bits = 0;<br>
<br>
// If it is fixed type, we only need to increase the FixedParamNum, for<br>
// the bit encode of fixed type is bit of zero, we do not need to change the<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>
</blockquote></div></div>
</blockquote></div></div>
</blockquote></div>
</blockquote></div></div>