Hi Tim,<div><br></div><div>Thanks for your review!</div><div><br></div><div>The generic code in getLoad is asserting (ValVT == MemVT)  for NON_EXTLOAD, so I think we should not only handle CCValAssign::BCvt, but all the other cases. Now I changed the code as attached by initializing MemVT to be ValVT along with ExtType initialized to NON_EXTLOAD. I think it makes sense to get those two initialization combined.</div>
<div><br></div><div>Yes, I can reproduce the failure exposed by "llc -debug", and it seems this issue is caused by the promotion from i8 to i32. Assert[SZ]Ext is implicating this breakage so we should fix the generic code accordingly, right?</div>
<div><br></div><div>Thanks,</div><div>-Jiangning<br>so we should fix the generic code accordingly, right?<br>Tim Northover <<a href="mailto:t.p.northover@gmail.com">t.p.northover@gmail.com</a>>于2014年5月30日星期五写道：<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Jiangning,<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">get an assertion failure with both current code and any version</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
that fixes your problem. I think it's harmless, as far as any</blockquote>
<br>
> Currently AAPCS64 random test is broken for VPR stack parameters, so this<br>
> small patch is to fix it.<br>
so we should fix the generic code accordingly, right?<br>
I wondered why this code looked so familiar. It's because I introduced<br>
the bug a few days ago, so thanks for looking at it!<br>
<br>
Anyway, I don't think this is the right fix. Your change means that<br>
the ExtLoad can never actually do any extending, which means things<br>
will go badly when the switch cases above trigger. It's slightly<br>
worrying that these cases don't show up in the existing regression<br>
tests; I think I'll add some.<br>
<br>
The path you're actually coming through here is that LocInfo is<br>
CCValAssign::BCvt, so you should probably check that and perform the<br>
bitcast in the switch just above (like in the register case). Then the<br>
existing code should create a NON_EXTLOAD without difficulty.<br>
<br>
Cheers.<br>
so we should fix the generic code accordingly, right?<br>
Tim.<br>
<br>
P.S.<br>
<br>
Actually, I think this area needs more work. I don't think the switch<br>
above ever triggers at the moment, but it really should because<br>
generic code in SelectionDAGBuilder.cpp asserts that<br>
LowerFormalArguments returns the type it's expecting. If you run "llc<br>
-debug" on this file:<br>
<br>
define i8 @foo([8 x i64], i8 %stack) {<br>
  ret i8 %stack<br>
}<br>
<br>
We get an assertion failure with both current code and any version<br>
that fixes your problem. I think it's harmless, as far as any<br>
assertion failure is, but we're breaking a contract.<br>
</blockquote></div>