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

Jiangning Liu liujiangning1 at gmail.com
Mon Jun 2 20:41:55 PDT 2014


Hi Tim,

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.

For your  fix around lowering i1, i8 and i16 to meet generic code
requirement, I'm not sure I'm understanding it correctly.

ValVT should be meaning the original type defined by end-user, while LocVT
means the type we promote to following ABI, correct?

-      ArgValue = DAG.getExtLoad(ExtType, DL, VA.getValVT(), Chain, FIN,
+      ArgValue = DAG.getExtLoad(ExtType, DL, VA.getLocVT(), Chain, FIN,
                                 MachinePointerInfo::getFixedStack(FI),
-                                VA.getLocVT(),
+                                VA.getValVT(),
                                 false, false, false, 0);

Your code change of switching parameter passing of ValVT and LocVT got me
confused.

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?

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.

       Arg = EmitIntExt(SrcVT, Arg, DestVT, /*isZExt*/ false);
       if (Arg == 0)
         return false;
-      ArgVT = DestVT;
       break;
     }
     case CCValAssign::AExt:
@@ -1229,7 +1228,6 @@ bool AArch64FastISel::ProcessCallArgs(
       Arg = EmitIntExt(SrcVT, Arg, DestVT, /*isZExt*/ true);
       if (Arg == 0)
         return false;
-      ArgVT = DestVT;
       break;
     }

Thanks,
-Jiangning


2014-06-03 0:19 GMT+08:00 Tim Northover <t.p.northover at gmail.com>:

> Hi Jiangning,
>
> > 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?
>
> I think the generic code is probably doing the right thing. AArch64 is
> using the CC stuff in some rather odd ways, and we shouldn't really
> expect generic code to be lax in its checks to accommodate us. We
> should be handing it an object of the type it's asked for.
>
> I've got a patch ready that fixes this in AArch64 code, but I was
> waiting for you to finish with this one so you don't have to deal with
> any conflicts from it. I've attached it here for you to have a look,
> though it's still unpoloshed. I *think* all that's needed in addition
> to handle your particular problem is a "case CCValAssign::BCvt:
> DAG.getNode(ISD::BITCAST, ...); break;".
>
> Cheers.
>
> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140603/65ddfd65/attachment.html>


More information about the llvm-commits mailing list