<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 10, 2014 at 4:02 PM, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.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">Hi Arnaud,<br>
<div class=""><br>
> For example, with this patch, the closest match for the assembler is the pseudo<br>
> in one of the basic-a64-diagnostics tests, which leads to a weird diagnostic. Would<br>
> there be a non convoluted way to disambiguate this, or give a lower priority to<br>
> pseudos ?<br>
<br>
</div>Not that I'm aware of. The diagnostic doesn't look too bad to me.<br>
There are certainly worse around in LLVM's assemblers.<br>
<br>
I don't think "pseudo-instruction" or related terms is right here<br>
though. LLVM actually has pseudo-instructions of various kinds, and<br>
they're not used in this patch (fortunately -- that would have been<br>
the wrong way to do it and I was a little worried before I opened up<br>
the attachment). </blockquote><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"> </blockquote><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">
+def logical_imm32_not_XFORM : SDNodeXForm<imm, [{<br>
<br>
SDNodeXForms only apply to CodeGen, but you're not actually changing<br>
any patterns so I think these are unnecessary. Similarly for the<br>
PatLeaf instantiations. Operands can be much more minimal if they're<br>
only used in InstAliases.<br></blockquote><div><br></div><div>Those codegen stuff definitely looked unnecessary to me, but I did not even try without those :(</div><div>I have removed the PatLeaf and XForms as well. <br>
</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">
+ let PrintMethod = "printLogicalImm32";<br>
<br>
I hope these are unnecessary too. Or do they get referred to in<br>
generated files even if they're not called?<br></blockquote><div><br></div><div>You're right. They are not necessary.</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">
+ string pseudo> {<br>
<br>
Following my first comments, I think this is the wrong name. Perhaps<br>
NegAlias or something? Even just "Alias" would be more accurate.<br></blockquote><div><br></div><div>Let's settle for Alias then.</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">
<br>
+ let AddedComplexity = 6 in<br>
<br>
With no new CodeGen patterns, this seems unnecessary. Also untested,<br>
since there aren't any .ll tests.<br>
<br></blockquote><div><br></div><div>I had to move it from ouside of the multiclass, to where it is used inside the multiclass, because InstAlias does not have those. As you point, the alias have no codegen pattern.</div>
<div><br></div><div>I have not changed the functionality there, so no additionnal .ll test is necessary -- I think.</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">
+ const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());<br>
+ assert(MCE && "Invalid logical immediate operand!");<br>
<br>
If you use "cast" instead of "dyn_cast" it includes an assert.<br></blockquote><div><br></div><div>Changed the patch to use cast instead.</div><div><br></div><div>By the way, this could applies in many places around. If you believe it is worth, than I can fix those other places (and refactor a bit) in a separate patch. This would make the code more readable.</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">
Other than that, it looks reasonable to me.<br></blockquote><div><br></div><div>There was quite a lot of fixes :(</div><div><br></div><div>Thanks for the quick review !</div><div><br></div><div>I attached the updated patch. I left the fixme note in basic-a64-diagnostics; I will remove it before committing if you think we just don't care. I also changed pseudo to alias in the commit message or comments.</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">
Cheers.<br>
<span class=""><font color="#888888"><br>
Tim.<br>
</font></span></blockquote></div><br></div></div>