<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 01/26/2015 04:57 PM, Fiona Glaser
wrote:<br>
</div>
<blockquote
cite="mid:8BA95C9E-7038-4C00-BE52-75EB0704C60D@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<br class="">
<div>
<blockquote type="cite" class="">
<div class="">On Jan 26, 2015, at 4:51 PM, Matt Arsenault <<a
moz-do-not-send="true"
href="mailto:Matthew.Arsenault@amd.com" class="">Matthew.Arsenault@amd.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<div class="moz-cite-prefix">On 01/26/2015 04:35 PM, Fiona
Glaser wrote:<br class="">
</div>
<blockquote
cite="mid:310CA9BE-7A33-463F-A5BC-7E3074A4FD91@apple.com"
type="cite" class="">
<div class="">On an out-of-tree target, IR similar to
the attached causes the emission of the following
instructions (pseudocode):</div>
<div class=""><br class="">
</div>
<div class="">setcc dst16, cc</div>
<div class="">zext dst32, dst16</div>
<div class=""><br class="">
</div>
<div class="">The attached patch fixes it to be the
expected:</div>
<div class=""><br class="">
</div>
<div class="">setcc dst32, cc</div>
<div class=""><br class="">
</div>
<div class="">because the out-of-tree target supports
32-bit setcc.</div>
</blockquote>
<br class="">
Does it also support 16-bit setcc result types or return
different sized types for different compared types? I
don't think this patch is generally correct. It isn't
correct to blindly promote the setcc type, especially with
any given promotion type. At the very least this would
only be valid to combine zext if the target's boolean
content is ZeroOrOneBooleanContent etc. setccs should only
be created with a type from getSetCCResultType, and any
random extended type you give it probably won't work<br
class="">
<br class="">
-Matt</div>
</div>
</blockquote>
<br class="">
</div>
<div>What’s the correct way to do this then? Should I only perform
the merge if the target supports the operation (i.e. never do it
to a non-legal type)? That feels slightly weird given that it
seemed like every other canonicalization done in that function
supports non-legal types, so I’d at least like to know what the
reason for that inconsistency is.</div>
<div><br class="">
</div>
<div>I was trying to model the optimization after the code that
immediately follows it, which already assumes that the setCC
resolves to a 0 or 1:</div>
<div><br class="">
</div>
<div>
<div style="margin: 0px; font-size: 11px; font-family: Menlo;"
class=""> // aext(setcc x,y,cc) -> select_cc x, y, 1,
0, cc</div>
<div style="margin: 0px; font-size: 11px; font-family: Menlo;"
class=""> SDValue SCC =</div>
<div style="margin: 0px; font-size: 11px; font-family: Menlo;"
class=""> SimplifySelectCC(SDLoc(N), N0.getOperand(0),
N0.getOperand(1),</div>
</div>
<div><br class="">
</div>
<div>Fiona</div>
</blockquote>
<br>
The aext -> select_cc here is OK, but in your patch it will
combine zext (setcc) -> setcc. On a target that uses
ZeroOrNegativeOneBooleanContent, that would produce a wider type
with 0 upper bits which is not the same as the all 1 bits the larger
compare result would give. There are also a lot of pre-existing bugs
that mishandle this now that still assume 0/1 setcc results.<br>
<br>
I guess I don't understand why this wouldn't already be happening
during legalization if getSetCCResultType gives i32. I would not
expect two different result types for the same setcc to ever work.<br>
<br>
-Matt<br>
<br>
</body>
</html>