[PATCH][AArch64] Fix bug around passing VPR stack parameters

Jiangning Liu liujiangning1 at gmail.com
Mon Jun 2 02:22:12 PDT 2014


Hi Tim,

Thanks for your review!

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.

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?

Thanks,
-Jiangning
so we should fix the generic code accordingly, right?
Tim Northover <t.p.northover at gmail.com>于2014年5月30日星期五写道:

> Hi Jiangning,
>>
>> get an assertion failure with both current code and any version
>
> that fixes your problem. I think it's harmless, as far as any
>
>
> > Currently AAPCS64 random test is broken for VPR stack parameters, so this
> > small patch is to fix it.
> so we should fix the generic code accordingly, right?
> I wondered why this code looked so familiar. It's because I introduced
> the bug a few days ago, so thanks for looking at it!
>
> Anyway, I don't think this is the right fix. Your change means that
> the ExtLoad can never actually do any extending, which means things
> will go badly when the switch cases above trigger. It's slightly
> worrying that these cases don't show up in the existing regression
> tests; I think I'll add some.
>
> The path you're actually coming through here is that LocInfo is
> CCValAssign::BCvt, so you should probably check that and perform the
> bitcast in the switch just above (like in the register case). Then the
> existing code should create a NON_EXTLOAD without difficulty.
>
> Cheers.
> so we should fix the generic code accordingly, right?
> Tim.
>
> P.S.
>
> Actually, I think this area needs more work. I don't think the switch
> above ever triggers at the moment, but it really should because
> generic code in SelectionDAGBuilder.cpp asserts that
> LowerFormalArguments returns the type it's expecting. If you run "llc
> -debug" on this file:
>
> define i8 @foo([8 x i64], i8 %stack) {
>   ret i8 %stack
> }
>
> We get an assertion failure with both current code and any version
> that fixes your problem. I think it's harmless, as far as any
> assertion failure is, but we're breaking a contract.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140602/a6f208dc/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: aapcs64_fix.patch
Type: text/x-patch
Size: 2184 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140602/a6f208dc/attachment.bin>


More information about the llvm-commits mailing list