<div dir="ltr">On 2nd thought, any time we have a transform loophole like this and we really want to do the opposite transform, we run the risk of infinite looping. Let me look at adding folds that start with sext/zext, so we know we're catching the trunc/select/ext cases exactly without needing this extra check.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 16, 2016 at 12:50 AM, Filipe Cabecinhas <span dir="ltr"><<a href="mailto:filcab+llvm.phabricator@gmail.com" target="_blank">filcab+llvm.phabricator@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yeah, a TODO is enough for now. This patch makes it strictly better,<br>
so I'm good with it.<br>
<br>
Thank you,<br>
 Filipe<br>
<div class="HOEnZb"><div class="h5"><br>
On Wed, Nov 16, 2016 at 12:09 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com">spatel@rotateright.com</a>> wrote:<br>
> spatel added inline comments.<br>
><br>
><br>
> ================<br>
> Comment at: lib/Transforms/InstCombine/<wbr>InstructionCombining.cpp:805<br>
> +  auto isNonTruncVariable = [](Value *V) {<br>
> +    return !isa<Constant>(V) && !match(V, m_Trunc(m_Value()));<br>
> +  };<br>
> ----------------<br>
> filcab wrote:<br>
>> Do you want to match type sizes, though? Or at least make sure you're truncating more (or the same) as you're extending?<br>
>> Like this:<br>
>> ```[build-debug]% cat | ./bin/opt -O3 - -o - -S<br>
>> define <4 x i64> @g3vec(<4 x i32> %_a, <4 x i1> %cmp) {<br>
>>   %a = trunc <4 x i32> %_a to <4 x i24><br>
>>   %sel = select <4 x i1> %cmp, <4 x i24> %a, <4 x i24> <i24 42, i24 42, i24 42, i24 42><br>
>>   %ext = zext <4 x i24> %sel to <4 x i64><br>
>>   ret <4 x i64> %ext<br>
>> }<br>
>><br>
>><br>
>> ; ModuleID = '<stdin>'<br>
>> source_filename = "<stdin>"<br>
>><br>
>> ; Function Attrs: norecurse nounwind readnone<br>
>> define <4 x i64> @g3vec(<4 x i32> %_a, <4 x i1> %cmp) local_unnamed_addr #0 {<br>
>>   %1 = and <4 x i32> %_a, <i32 16777215, i32 16777215, i32 16777215, i32 16777215><br>
>>   %2 = zext <4 x i32> %1 to <4 x i64><br>
>>   %ext = select <4 x i1> %cmp, <4 x i64> %2, <4 x i64> <i64 42, i64 42, i64 42, i64 42><br>
>>   ret <4 x i64> %ext<br>
>> }<br>
>><br>
>> attributes #0 = { norecurse nounwind readnone }<br>
>> ```<br>
>> vs just `select` + `zext` (using `sext` will make it even worse :-)<br>
>><br>
> This case would be another improvement over the current behavior, right? Ok if I add a 'TODO' comment in this patch and follow up with another test case plus that refinement?<br>
><br>
><br>
> <a href="https://reviews.llvm.org/D26556" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26556</a><br>
><br>
><br>
><br>
</div></div></blockquote></div><br></div>