<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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 class=""><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 class="">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 class=""><br></span></blockquote><div><br></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><div><br></div><div>David</div><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 class="">
<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 class="">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 class=""><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 class="">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 class=""><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 class="">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><br></div></div>