<div dir="ltr"><div>David,</div><div><br></div><div>I tried again to do without ReplaceValueWith in SoftenFloatResult for fp128,</div><div>and with cloning of nodes in SoftenFloatOperand. I found more problems than</div><div>my previous FCOPYSIGN example.</div><div><br></div><div>For the CopyToReg opcode, I found multiple formats and</div><div>there could be multiple "result values". The simple call to</div><div> ReplaceValueWith(SDValue(N, 0), Res);</div><div>at the end of current SoftenFloatOperand won't work.</div><div>I will need to call</div><div> ReplaceValueWith(SDValue(N, i), SDValue(NewNode, i));</div><div>for every result i.</div><div>So that added not only more code into SoftenFloatOp_*, but also</div><div>more complicated SoftenFloatOperand.</div><div><br></div><div>Another more difficult pattern is </div><div> Soften float result 0: t4: f128 = fmul t2, ConstantFP:f128</div><div>followed by</div><div> Expand integer result: t5: i128 = bitcast t4</div><div><br></div><div>If SoftenFloatResult does not call ReplaceValueWith for t4,</div><div>ExpandIntegerResult will need to "replace" its operand with softened t4</div><div>or clone the whole BITCASE node. That will require more complicated changes in ExpandRes_BITCAST(N, Lo, Hi) and ExpandIntegerResult.</div><div>Right now, in my patch I have only a simple condition inside ExpandRes_BITCAST<br></div><div>to do nothing for fp128, because we can keep f128 and i128 in the same register.</div><div><br></div><div>So, without ReplaceValueWith inside SoftenFloatResult, we not only need to</div><div>clone nodes in SoftenFloatOperands, but also in other kinds of "legalize"</div><div>functions like ExpandIntegerResult.</div><div><br></div><div>Comparing the two approaches:</div><div>(1) calling ReplaceValueWith of a node to save cloning of all user nodes like I have now, or</div><div>(2) not calling ReplaceValueWith of a node but clone all user nodes when that node is scanned as an operand.</div><div>I saw both patterns in llvm, but (1) seemed to be more efficient.<br></div><div>I think the <valid>Op_* functions exist because some operand nodes are not transformed</div><div>to other "legal" type. Right now most "illegal" type node are transformed to legal type,</div><div>but that won't be true for new x86_64 f128 type. If we use approach (2), we will need to add many new <valid>Op_* functions and even change the ExpandIntegerResult function.</div><div>To me, it is lower risk to depend on fewer <valid>Op_* functions to handle f128/i128 values in register, than to depend on more <valid>Op_* and <valid>Res_* functions to clone the new soften-yet-still-f128-type operands.</div><div><br></div><div>Beside this issue, do you see other area to change?</div><div><br></div><div>I have tried the part 1 subset mentioned in my previous comment.</div><div>That passed all current llvm unit tests and include the following files.</div><div> include/llvm/Target/TargetLowering.h</div><div> include/llvm/Target/TargetRegisterInfo.h</div><div> lib/CodeGen/SelectionDAG/DAGCombiner.cpp</div><div> lib/CodeGen/SelectionDAG/InstrEmitter.cpp</div><div> lib/CodeGen/SelectionDAG/LegalizeDAG.cpp</div><div> lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp</div><div> lib/CodeGen/SelectionDAG/LegalizeTypes.cpp</div><div> lib/CodeGen/SelectionDAG/LegalizeTypes.h</div><div> lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp</div><div> lib/CodeGen/SelectionDAG/SelectionDAG.cpp</div><div> lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp</div><div> lib/CodeGen/SelectionDAG/TargetLowering.cpp</div><div> lib/CodeGen/TargetLoweringBase.cpp</div><div> lib/CodeGen/TargetRegisterInfo.cpp</div><div> lib/Target/X86/X86MCInstLower.cpp</div><div><br></div><div>If there is no other change, I will upload that part 1 change in a new patch.</div><div>Thanks.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 25, 2015 at 4:25 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 Tue, Nov 24, 2015 at 5:22 PM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>chh added a comment.<br>
<br>
David,<br>
<br>
</span>I will upload a new patch with some of your older suggested changes first.<br>
Please see details in the new code diff and upload comment.<br>
<br>
I met many difficulties implementing your latest inline comments<br>
in LegalizeFloatTypes.cpp, so there is no change with that idea yet.<br>
Please see my reply to the inline comment.<br>
<br>
For separating this large patch, I have not tried yet,<br>
but it might be okay to split at least into two parts:<br>
(2) The new unit test files *.ll and X86CallingConv.td, X86ISelLowering.cpp, X86InstrInfo.td, X86InstrSSE.td, X86RegisterInfo.td.<br>
(1) All the other files.<br>
<br>
We should be able to check in (1) and expect no change to all targets.<br>
Patch (2) will fix Android x86_64 problem, but still no change to other targets.<br>
I am assuming that we want some time to test (1) before submitting (2).<br>
Further split of (1) might be possible, but that would take more review<br>
and test cycles if there is little regression. When there is any regression,<br>
it should be easy to revert all changes in (1) and then debug-and-retry, right?<br>
I don't see a way to split (2), as we don't want to have the Android target<br>
in a new incomplete state different from the current one.<br>
<span><br>
<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>
> I don't quite like the short cut here either. The original architected flow is reasonable to me: basically<br>
> 1) After softenfloat result, the old SDValue is mapped to the new result<br>
> 2) the worklist based loop follow DU and use instrctions of the old result get processed -- operands are scanned.<br>
> 3) during scanning, the use of the old value will be replaced with the new value.<br>
> 4) if scan operands result in a new node, replaceValueWith then needs to be eagerly called here on demand.<br>
><br>
> I prefer you do not change the flow in SoftenFloatOperands and make a patch here to workaround it. The resulting change may look bigger (as more opcodes need to be handled), but it is what it is.<br>
><br>
> I also suggest you extract the following patches out seperately<br>
> 1) softenFloatResult and softenFloatOperands<br>
> 2) f128 constantFP<br>
> 3) register class small refactoring<br>
> 4) the rest (may also be breakable )<br>
</span>The difficulties I found in the current architecture are<br>
(1) Avoid duplicating a lot of code from SoftenFloatRes_* to SoftenFloatOp_*.<br>
(2) Make sure the changes in multiple SoftenFloatRes_* and SoftenFloatOp_* will be fixed correctly at the same time in the future.<br>
<br>
In SoftenFloatOperand, we can see that it calls ReplaceValueWith<br>
at the end for all kinds of opcode. The number of calls to ReplaceValueWith<br>
is not as many as we expected. Hence, it seems to me the norm is to share<br>
code at higher level than pushing down into SoftenFloatOp_* and SoftenFloatRes_*.<br>
It seems reasonable to call ReplaceValueWith at the end of<br>
SoftenFloatOperand and SoftenFloatResult.<br></blockquote><div><br></div><div><br></div></div></div><div>You can't compare SoftenFloatOperand with SoftenFloatResult like this. SoftenFloatResult is for the value producer (defs) -- there is no need eagerly call ReplaceValueWith because their consumer nodes will naturally follow and be processed (either as value producer or consumer with operand scanned).</div><div><br></div><div>SoftenFloatOperands on the other hand, by definition, does not enable new consumer nodes to be ready in the worklist so it needs to call ReplaceValueWith when the node changes.</div><div><br></div><div>Anyway, I think we are getting close to a closure. Can you first carve out part-1 (as I proposed above) first (I assume it won't affect others).</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I tried to remove this part, but the result was either<br>
(a) calling ReplaceValuewith inside SoftenFloatRes_*, or<br>
(b) copying many SoftenFloatRes_* code into SoftenFloatOp_* and then call ReplaceValueWith in SoftenFloatOp_*. The copied code is not all simple and small. For example, SoftenFloatRes_FCOPYSIGN is complicated, and I don't see how simpler it is to duplicate it into SoftenFloatOp_FCOPYSIGN, or abstract it out into a shared function.<br>
<br>
What you mentioned in another email earlier might be better to<br>
move some SoftenFloatOp_* responsibility into SoftenFloatRes_*.<br>
Then, we have less code duplication, and we need to make sure<br>
that ReplaceValueWith is called whenever something is changed<br>
by SoftenFloatRes_*. I could try that when I have more time.<br>
<br>
My concern (2) is more scaring. During my test of those code movement,<br>
I made a few mistakes and the compiler could run and generate inefficient<br>
code because some ReplaceValueWith call was missed or some fp128 was<br>
not kept in register due to a short-cut was missed. So I would prefer<br>
a way easier and smaller change to verify no no-impact to existing targets,<br>
by conditioned on isLegalInHWReg, even if some can be applied to other targets.<br>
<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></span></div><br></div></div>
</blockquote></div><br></div>