<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 24, 2015 at 7:39 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.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="HOEnZb"><div class="h5">> On 2015-Feb-24, at 15:58, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> ping<br>
><br>
> On Sat, Feb 14, 2015 at 9:39 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> Hi rafael, resistor, grosbach, chandlerc, grosser,<br>
><br>
> Essentially the same as the GEP change under review in<br>
> <a href="http://reviews.llvm.org/D7636" target="_blank">http://reviews.llvm.org/D7636</a><br>
><br>
> A similar migration script can be used to update test cases, though a few more<br>
> test case improvements/changes were required this time around: (r229269-r229278)<br>
><br>
> import fileinput<br>
> import sys<br>
> import re<br>
><br>
> pat = re.compile(r"((?:=|:|^)\s*load (?:atomic )?(?:volatile )?(.*?))(| addrspace\(\d+\) *)\*($| *(?:%|@|null|undef|blockaddress|getelementptr|addrspacecast|bitcast|inttoptr|\[\[[a-zA-Z]|\{\{).*$)")<br>
><br>
> for line in sys.stdin:<br>
>   sys.stdout.write(re.sub(pat, r"\1, \2\3*\4", line))<br>
><br>
> <a href="http://reviews.llvm.org/D7649" target="_blank">http://reviews.llvm.org/D7649</a><br>
><br>
> Files:<br>
>   lib/AsmParser/LLParser.cpp<br>
>   lib/IR/AsmWriter.cpp<br>
><br>
> Index: lib/AsmParser/LLParser.cpp<br>
> ===================================================================<br>
> --- lib/AsmParser/LLParser.cpp<br>
> +++ lib/AsmParser/LLParser.cpp<br>
> @@ -5165,6 +5165,10 @@<br>
>      Lex.Lex();<br>
>    }<br>
><br>
> +  Type *Ty = nullptr;<br>
> +  if (ParseType(Ty)) return true;<br>
> +  ParseToken(lltok::comma, "expected comma after getelementptr's type");<br>
> +<br>
<br>
</div></div>This should be:<br>
<br>
    if (ParseType(Ty) ||<br>
        ParseToken(lltok::comma, "expected comma after load's type"))<br>
      return true;<br>
<br>
(Changed the message and checked the return of ParseToken().)<br>
<br>
I think a testcase for this diagnostic is fairly important, since<br>
it'll be what rejects the old syntax.  I'd suggest something like<br>
test/Assembler/invalid-load-missing-explicit-type.ll.<br></blockquote><div><br>Diagnostic fixed, ParseToken result checked/returned, test case added.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>    if (ParseTypeAndValue(Val, Loc, PFS) ||<br>
>        ParseScopeAndOrdering(isAtomic, Scope, Ordering) ||<br>
>        ParseOptionalCommaAlign(Alignment, AteExtraComma))<br>
> @@ -5179,6 +5183,9 @@<br>
>      return Error(Loc, "atomic load cannot use Release ordering");<br>
><br>
>    Inst = new LoadInst(Val, "", isVolatile, Alignment, Ordering, Scope);<br>
> +<br>
> +  assert(Ty == Inst->getType());<br>
<br>
</span>This should be an `Error()` with a diagnostic, not an `assert()`, since<br>
this is reachable via:<br>
<br>
    %reg = load i32, i8* %mem<br>
<br>
(Or isn't it?)<br>
<br>
Assuming I'm right, a testcase for this error would be useful as well.<br></blockquote><div><br>Changed assert to error handling and tested.<br> </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>
> +<br>
>    return AteExtraComma ? InstExtraComma : InstNormal;<br>
>  }<br>
><br>
> Index: lib/IR/AsmWriter.cpp<br>
> ===================================================================<br>
> --- lib/IR/AsmWriter.cpp<br>
> +++ lib/IR/AsmWriter.cpp<br>
> @@ -2880,6 +2880,11 @@<br>
>      Out << ", ";<br>
>      TypePrinter.print(I.getType(), Out);<br>
>    } else if (Operand) {   // Print the normal way.<br>
> +    if (auto *LI = dyn_cast<LoadInst>(&I)) {<br>
> +      Out << ' ';<br>
> +      TypePrinter.print(LI->getType(), Out);<br>
> +      Out << ", ";<br>
> +    }<br>
><br>
>      // PrintAllTypes - Instructions who have operands of all the same type<br>
>      // omit the type from all but the first operand.  If the instruction has<br>
><br>
> EMAIL PREFERENCES<br>
>   <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
> _______________________________________________<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>
><br>
> _______________________________________________<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>