<div dir="ltr">Hi Tim,<div><br></div><div>I don't want to block your progress of cleaning the conflict with generic parameter lowering code, so now I've committed my patch as r210067. You can just change whatever you want to merge with your new fix to solve that conflict with generic code.</div>
<div><br></div><div>For your  fix around lowering i1, i8 and i16 to meet generic code requirement, I'm not sure I'm understanding it correctly. </div><div><br></div><div>ValVT should be meaning the original type defined by end-user, while LocVT means the type we promote to following ABI, correct?</div>
<div><br></div><div><div>-      ArgValue = DAG.getExtLoad(ExtType, DL, VA.getValVT(), Chain, FIN,</div><div>+      ArgValue = DAG.getExtLoad(ExtType, DL, VA.getLocVT(), Chain, FIN,</div><div>                                 MachinePointerInfo::getFixedStack(FI),</div>
<div>-                                VA.getLocVT(),</div><div>+                                VA.getValVT(),</div><div>                                 false, false, false, 0);</div></div><div><br></div><div>Your code change of switching parameter passing of ValVT and LocVT got me confused.</div>
<div><br></div><div>I think the memory load here should be from the pointer type of LocVT rather than ValVT, and the data we stored to register should be the type ValVT, and then the code inside the function should just use ValVT, right?</div>
<div><br></div><div>Instead, the the following fix in call site lowering looks good to me and it would keep the original type ValVT, and store it to LocVT pointer.</div><div><br></div><div><div>       Arg = EmitIntExt(SrcVT, Arg, DestVT, /*isZExt*/ false);</div>
<div>       if (Arg == 0)</div><div>         return false;</div><div>-      ArgVT = DestVT;</div><div>       break;</div><div>     }</div><div>     case CCValAssign::AExt:</div><div>@@ -1229,7 +1228,6 @@ bool AArch64FastISel::ProcessCallArgs(</div>
<div>       Arg = EmitIntExt(SrcVT, Arg, DestVT, /*isZExt*/ true);</div><div>       if (Arg == 0)</div><div>         return false;</div><div>-      ArgVT = DestVT;</div><div>       break;</div><div>     }</div></div><div>
<br></div><div>Thanks,</div><div>-Jiangning</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-03 0:19 GMT+08:00 Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jiangning,<br>
<div class=""><br>
> Yes, I can reproduce the failure exposed by "llc -debug", and it seems this<br>
> issue is caused by the promotion from i8 to i32. Assert[SZ]Ext is<br>
> implicating this breakage so we should fix the generic code accordingly,<br>
> right?<br>
<br>
</div>I think the generic code is probably doing the right thing. AArch64 is<br>
using the CC stuff in some rather odd ways, and we shouldn't really<br>
expect generic code to be lax in its checks to accommodate us. We<br>
should be handing it an object of the type it's asked for.<br>
<br>
I've got a patch ready that fixes this in AArch64 code, but I was<br>
waiting for you to finish with this one so you don't have to deal with<br>
any conflicts from it. I've attached it here for you to have a look,<br>
though it's still unpoloshed. I *think* all that's needed in addition<br>
to handle your particular problem is a "case CCValAssign::BCvt:<br>
DAG.getNode(ISD::BITCAST, ...); break;".<br>
<br>
Cheers.<br>
<span class="HOEnZb"><font color="#888888"><br>
Tim.<br>
</font></span></blockquote></div><br></div>