<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Robert,<div><br></div><div><div><div>On Aug 7, 2014, at 4:04 AM, Robert Khasanov <<a href="mailto:rob.khasanov@gmail.com">rob.khasanov@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Hi Adam, <div><br></div><div><br></div><div>I have some notes to your change.</div><div><br></div><div><div>* Some instructions have masking versions, but don't have zero-masking (e.g. stores and other write memory instructions). I think there are two different ways to take into account: </div>
<div> 1) make separate multiclasses for (r +rk + rkz) and (r+rk); </div><div> 2) Propose new feature to TableGen to create conditional def/defm in multiclasses. This feature will be suitable also for embedded broadcasts: only instructions w/ 32 and 64 bit element size have embedded broadcast feature. However, I don't know how it can look in syntax, I have just an idea and I'm not sure the community will accept this.</div></div></div></blockquote><div><br></div><div>I have been successful using multiclass inheritance for this. Take a look at avx512_perm_table_3src for example which derives from avx512_perm_3src only to add the intrinsics (the idea being that for ). Sounds like the same approach would work for the cases you mention.</div><br><blockquote type="cite"><div dir="ltr"><div>
</div><div>* There are some instructions that have not lowering patterns from LLVM IR instructions. I suggest to use !if(x,[(set RC:$dst, RHS)],[]) where x is some condition or bit that is passed from multiclass arg. </div></div></blockquote><div><br></div><div>Agreed.</div><br><blockquote type="cite"><div dir="ltr"><div>You can fix last item in this patch or later in separate patch. </div></div></blockquote><div><br></div><div>I will do that when adding an instance of such instruction.</div></div><div><br><blockquote type="cite"><div dir="ltr"><div>LGTM</div></div></blockquote><div><br></div><div>Thanks for the review! Committed as r215125.</div><div><br></div><div>Adam</div><br><blockquote type="cite"><div dir="ltr"><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-08-07 9:28 GMT+04:00 Adam Nemet <span dir="ltr"><<a href="mailto:anemet@apple.com" target="_blank">anemet@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
After adding the masking variants to several instructions, I have decided to<br>
experiment with generating these from the non-masking/unconditional<br>
variant. This will hopefully reduce the amount repetition that we currently<br>
have in order to define an instruction with all its variants (for a reg/mem<br>
instruction this would be 6 instruction defs and 2 Pat<> for the intrinsic).<br>
<br>
The patch is the first cut that is currently only applied to valignd/q to keep<br>
the patch small.<br>
<br>
A few notes on the approach:<br>
<br>
* In order to stitch together the dag for both the conditional and the<br>
unconditional patterns I pass the RHS of the set rather than the full<br>
pattern (set dest, RHS).<br>
* Rather than subclassing each instruction base class (e.g. AVX512AIi8),<br>
with a masking variant which wouldn't scale, I derived the masking<br>
instructions from a new base class AVX512 (this is just I<> with<br>
Requires<HasAVX512>). The instructions derive from this now, plus a new set<br>
of classes that add the format bits and everything else that instruction<br>
base class provided (i.e. AVX512AIi8 vs. AVX512AIi8Base).<br>
<br>
I hope we can go incrementally from here. I expect that:<br>
<br>
* We will need different variants of the masking class. One example is<br>
instructions requiring three vector sources. In this case we tie one of the<br>
source operands to dest rather than a new implicit source operand ($src0)<br>
* Add the zero-masking variant<br>
* Add more AVX512*Base classes as new uses are added<br>
<br>
I've looked at X86.td.expanded before and after to make sure that nothing got<br>
lost for valignd/q.<br>
<br>
Please let me know if it looks good.<br>
<span class="HOEnZb"><font color="#888888"><br>
Adam<br>
<br>
</font></span></blockquote></div><br></div>
</blockquote></div><br></div></body></html>