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