<div dir="ltr"><div>David,</div><div><br></div><div>Although I said that ScanOperands after the legalizing-result-type loop is strange and might be inefficient, I feel that it is safer and simpler to reuse existing flow than embedding some legalization of operands into the legalize-result-type functions.</div><div><br></div><div>I don't think the current design assumes all operands to be "legal" after the legalizing-result-type loop. The operands are required to be legal only when the node is "changed". When IgnoredNodeRsults(N) is true, or when the type action is TypeLegal, ScanOperands is executed.</div><div><br></div><div>Your fix is similar to embedding PromoteIntOp_SELECT into SoftenFloatRes_SELECT.</div><div>In that case, I think GetPromotedInteger(Cond) will work only if that Cond value was promoted before. What needed is probably a call to PromoteIntOp_SELECT inside SoftenFloatRes_SELECT.</div><div><br></div><div>But, how do we know that SoftenFloatRes_SELECT's "illegal" operand could only be integers to be promoted? Could it require expand or soften? I don't see other Op_SELECT now, but if a new illegal SELECT operand type is added later, we will have to add that to multiple places. So, embedding *Op_XYZ into *Res_XYZ now will require a check of all possible type actions of the operand.</div><div><br></div><div>SELECT_CC is more complicated. I see 5 kinds of Op_SELECT_CC functions now:</div><div> PromoteIntOp_SELECT_CC</div><div> ExpandIntOp_SELECT_CC</div><div> SoftenFloatOp_SELECT_CC</div><div> ExpandFloatOp_SELECT_CC</div><div> PromoteFloatOp_SELECT_CC</div><div><br></div><div>So it seems safer and less repeated code now to reuse the ScanOperands loop.</div><div>We only need to return false for the "changed" flag from SoftenFloatResult</div><div>or any legalizing-result-type function.</div><div>Does that make sense to you?</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 13, 2015 at 5:53 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Thu, Nov 12, 2015 at 11:06 AM, Chih-Hung Hsieh <span dir="ltr"><<a href="mailto:chh@google.com" target="_blank">chh@google.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">chh marked 2 inline comments as done.<br>
chh added a comment.<br>
<br>
Will upload diff 11 soon with suggested changes.<br>
<span><br>
<br>
================<br>
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:70<br>
@@ +69,3 @@<br>
+ case ISD::BITCAST:<br>
+ case ISD::SELECT:<br>
+ case ISD::SELECT_CC:<br>
----------------<br>
</span><span>davidxl wrote:<br>
> I got curious why these opcodes need to be skipped and picked ISD::SELECT to look at.<br>
><br>
> Commented line 70 out does trigger error -- but further debugging indicate the bug can be in the original code. In particular, PromoteIntRes_SETCC should call ReplaceValueWith, but does not.<br>
><br>
> Fixing SETCC handling makes the test case TestMax pass.<br>
><br>
> Can you verify this is a bug triggered by f128 change?<br>
</span>Yes, I can reproduce that error too. I believe that it happens only with<br>
my new x86_64 f128 configuration, but I have no way to verify for all targets<br>
and illegal types. It's arguably a "bug" as it worked with existing ScanOperands.<br>
<br>
However, the reason to skip some Opcode is not to workaround the problem of<br>
missed calls to ReplaceValueWith. We want to keep some Opcode with f128 type<br>
for instruction selection to correct calling convention or more efficient code.<br>
When we do not change SDNode for f128 type, we need to tell LegalizeTypes to<br>
treat it as a "legal" type and go to ScanOperands, so all other existing cases<br>
will work with this unchanged SDNode.<br>
<span><br></span></blockquote><div><br></div></div></div><div>I tried it for arm with soft-fp and see why it works in that case (i.e., without calling ReplaceValueWith). What happens when SoftenFloatRes_SELECT is called, a new SDNode is created due to f32->i32. The new node will then be added to the worklist and re-analyzed. When it is re-analyzed, the unpromoted integer (condition) operand of SELECT gets processed and it works fine.</div><div><br></div><div>However with f128, since f128 is kept in HW register, the SoftenFloatRes_SELECT ends up returning the original node due to CSE. This node does not get reanalyzed and one of its operand is an unpromoted integer thus causes later assertion.</div><div><br></div><div>If the current design assumes after SoftenFloatRes_OPCODE call, all the operands of the OPCODE node are Type Legalized, then the real bug is in SoftenFloatRes_SELECT function. I tried a simple fix (similar to other places) and it works just fine:</div><div><br></div><div><div>- return DAG.getSelect(SDLoc(N),</div><div>- LHS.getValueType(), N->getOperand(0), LHS, RHS);</div><div>+ SDValue Cond = N->getOperand(0);</div><div>+ if (getTypeAction(Cond.getValueType()) == TargetLowering::TypePromoteInteger)</div><div>+ Cond = GetPromotedInteger(Cond);</div><div>+ return DAG.getSelect(SDLoc(N),</div><div>+ LHS.getValueType(), Cond, LHS, RHS);</div></div><div><br></div><div>I think this fix should be safe for general case -- there is no need to skip the call to SoftenFloatRes_SELECT to workaround the problem.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><div><div class="h5"><div><br></div><div><br></div><div><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"><span>
<br>
================<br>
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:158<br>
@@ +157,3 @@<br>
+ // When LegalInHWReg is true, we take a short-cut in SoftenFloatOperand<br>
+ // to save repeated computations and make the code simpler there.<br>
+ // So here we need to call ReplaceValueWith.<br>
----------------<br>
</span><span>davidxl wrote:<br>
> Where is the short-cut? Is it the new default handling in SoftenFloatOperand? Making short cut there and have weak handshaking seems fragile.<br>
</span>Yes, in SoftenFloatOperand, I do not try to repeatedly replace a SDNode<br>
when its operand type can be kept as float and not changed.<br>
Current LegalizeTypes and LegalizeFloatTypes depend on ScanOperands to<br>
replace SDNode where DAGTypeLegalizer::*Result functions "missed" calls<br>
to ReplaceValueWith. That is kind of strange and somewhat inefficient.<br>
<br>
However, changing that architecture is too big a risk and work for me to<br>
fix x86_64 f128 calling convention and make sure the change work for<br>
all targets now and in the future. I don't think there are enough regression<br>
tests for all target and types that depend on LegalizeTypes. I found all broken<br>
cases when trying to compile libm on Android.<br>
<br>
Hence, I use this safer and "smaller" patch to match existing architecture<br>
and limit the impact to only SoftenFloat* and LegalInHWReg cases.<br>
It could be a separate patch to clean up the complicated loops inside LegalizeTypes,<br>
or LegalizeTypes could be rewritten as we know that there are non-HW-supported operations<br>
on some types but not really "illegal types".<br>
<span><br>
<br>
================<br>
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:752<br>
@@ +751,3 @@<br>
+ // Assume all child nodes were softened if changed in place.<br>
+ if (LegalInHWReg)<br>
+ return false;<br>
----------------<br>
</span><span>davidxl wrote:<br>
> What are the new opcodes that reach here? Please make such opcode explict and add assertions for unexpected ones.<br>
</span>Unspecified cases will dump and abort as before.<br>
<span><br>
<br>
================<br>
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:791<br>
@@ -711,2 +790,3 @@<br>
<br>
- ReplaceValueWith(SDValue(N, 0), Res);<br>
+ if (N != Res.getNode())<br>
+ ReplaceValueWith(SDValue(N, 0), Res);<br>
----------------<br>
</span><span>davidxl wrote:<br>
> This seems redundant -- see above early return.<br>
</span>Fixed.<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D11438" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11438</a><br>
<br>
<br>
<br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>