llvm dev - Jul 2019 - About a new porting of GlobalIsel for RISCV

Hi,
I would like to start a new porting of GlobalIsel for RISCV.
An initial patch about GlobalIsel infrastructure for RISCV was ready now:
 https://reviews.llvm.org/D65219
There is another porting patch https://reviews.llvm.org/D41653  posted
by Leslie Zhai at the end of 2017. I have checked with Leslie about
the status of this patch.He has stopped developing it since some
questions need be resolved. There are some discussions about it:
http://lists.llvm.org/pipermail/llvm-dev/2018-January/120098.html
http://lists.llvm.org/pipermail/llvm-dev/2018-August/125094.html
Till now, I think we have a reasonable solution to continue the work,
the implementation of GlobalIsel from Mips is a good example, which
use target-specific "MipsCCState" and
"MipsCallLowering::MipsHandler"
to handle Call/Arguments/Return lowering.
For RISCV, there's no "CCAssignFnForCall" or
"CCAssignFnForReturn"
functions defined, just like the solution in Mips, a new
target-specific "ValueHandler" will be created to support
calllowering.
I have made some experiment that trying to implement the "LowerReturn"
function, and it can return correctly. The code snippet may be as
follows:
  ...
  CCState CCInfo(F.getCallingConv(), F.isVarArg(), MF, ArgLocs, F.getContext());
  TLI.analyzeOutputArgs(MF, CCInfo, Outs, true, nullptr);
  RISCVValueHandler RetHandler(MIRBuilder, MF.getRegInfo(), Ret);
  RetHandler.handleArg(ArgLocs, RetInfos);
  ...
In order to reduce duplicated code as much as possible, and reuse part
of code from "TargetLowering" for GlobalIsel, the access specifiers
for some functions in RISCVTargetLowering need be changed, like
"analyzeOutputArgs". I have made a separate patch for it:
https://reviews.llvm.org/D65218
Since Leslie is no longer working on GlobalIsel, I want to continue
his work and make some contributions for RISCV target.
The code base was changed a lot from 2017, so I restart a new
GlobalIsel(RISCV) patch for reviewing, any suggestions or reviewing
comments would be appreciated.

Thanks,
Andrew Wei

Hi Andrew,

I've reviewed https://reviews.llvm.org/D65219 and LGTM'd it as it looks
like the normal boilerplate to me.

It looks like the main issue is the CallLowering side which unfortunately I
don't know much about as I haven't dug into that on the GISel side. I
would like to discourage CallLowering::ValueHandler replacements like
MipsCallLowering::MipsHandler unless there's a strong reason for them though
(subclassing CallLowering::ValueHandler is fine). As far as I can see
MipsCallLowering::MipsHandler generally doesn't need to have different
API's to CallLowering::ValueHandler and I'd say that all targets should
generally be using the same interfaces.

For the sake of completeness the MipsCallLowering::MipsHandler API changes I saw
were:
getStackAddress() takes a CCValAssign instead of Size and Offset but only
actually used the Size and Offset. The calling code could just pass them in.
assignValueToReg() is mostly different because there's a FPR->GPR
specific copy instruction. I never got around to working on it when I worked for
MIPS but I think that ideally there'd just be COPY and the instruction
differences would be handled by copyPhysReg() based on the reg classes for the
src and dst.
assignValueToAddress() removed three of the arguments but it could just ignore
them.
> On Jul 24, 2019, at 09:47, wei wei via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi,
> I would like to start a new porting of GlobalIsel for RISCV.
> An initial patch about GlobalIsel infrastructure for RISCV was ready now:
> https://reviews.llvm.org/D65219
> There is another porting patch https://reviews.llvm.org/D41653  posted
> by Leslie Zhai at the end of 2017. I have checked with Leslie about
> the status of this patch.He has stopped developing it since some
> questions need be resolved. There are some discussions about it:
> http://lists.llvm.org/pipermail/llvm-dev/2018-January/120098.html
> http://lists.llvm.org/pipermail/llvm-dev/2018-August/125094.html
> Till now, I think we have a reasonable solution to continue the work,
> the implementation of GlobalIsel from Mips is a good example, which
> use target-specific "MipsCCState" and
"MipsCallLowering::MipsHandler"
> to handle Call/Arguments/Return lowering.
> For RISCV, there's no "CCAssignFnForCall" or
"CCAssignFnForReturn"
> functions defined, just like the solution in Mips, a new
> target-specific "ValueHandler" will be created to support
> calllowering.
> I have made some experiment that trying to implement the
"LowerReturn"
> function, and it can return correctly. The code snippet may be as
> follows:
>  ...
>  CCState CCInfo(F.getCallingConv(), F.isVarArg(), MF, ArgLocs,
F.getContext());
>  TLI.analyzeOutputArgs(MF, CCInfo, Outs, true, nullptr);
>  RISCVValueHandler RetHandler(MIRBuilder, MF.getRegInfo(), Ret);
>  RetHandler.handleArg(ArgLocs, RetInfos);
>  ...
> In order to reduce duplicated code as much as possible, and reuse part
> of code from "TargetLowering" for GlobalIsel, the access
specifiers
> for some functions in RISCVTargetLowering need be changed, like
> "analyzeOutputArgs". I have made a separate patch for it:
> https://reviews.llvm.org/D65218
> Since Leslie is no longer working on GlobalIsel, I want to continue
> his work and make some contributions for RISCV target.
> The code base was changed a lot from 2017, so I restart a new
> GlobalIsel(RISCV) patch for reviewing, any suggestions or reviewing
> comments would be appreciated.
> 
> Thanks,
> Andrew Wei
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Hi, Daniel,
Thank you for your comments and suggestions.
I think you're right, we'd better not replace the existing
"ValueHandler" with target-specific ones.
The biggest problems I encountered was that in RISCV there's no
"CCAssignFnForCall" and "CCAssignFnForReturn" interface,
which will be
one of the arguments for constructor function(ValueHandler). But it
seems this problem can be solved. After I implemented some APIs from
"CallLowering::ValueHandler" like "assignArg" and
"assignValueToReg",
several simple cases can run successfully.
Currently, I need to do some experiments to verify the feasibility of
this method, and try to write as many cases as possible to cover
different scenarios. I am confident that I can complete a basic
implementation of GlobalIsel Calllowering for RISCV.

Thanks,
Andrew Wei

Daniel Sanders <daniel_l_sanders at apple.com> 于2019年7月26日周五
上午4:00写道：
>
> Hi Andrew,
>
> I've reviewed https://reviews.llvm.org/D65219 and LGTM'd it as it
looks like the normal boilerplate to me.
>
> It looks like the main issue is the CallLowering side which unfortunately I
don't know much about as I haven't dug into that on the GISel side. I
would like to discourage CallLowering::ValueHandler replacements like
MipsCallLowering::MipsHandler unless there's a strong reason for them though
(subclassing CallLowering::ValueHandler is fine). As far as I can see
MipsCallLowering::MipsHandler generally doesn't need to have different
API's to CallLowering::ValueHandler and I'd say that all targets should
generally be using the same interfaces.
>
> For the sake of completeness the MipsCallLowering::MipsHandler API changes
I saw were:
> getStackAddress() takes a CCValAssign instead of Size and Offset but only
actually used the Size and Offset. The calling code could just pass them in.
> assignValueToReg() is mostly different because there's a FPR->GPR
specific copy instruction. I never got around to working on it when I worked for
MIPS but I think that ideally there'd just be COPY and the instruction
differences would be handled by copyPhysReg() based on the reg classes for the
src and dst.
> assignValueToAddress() removed three of the arguments but it could just
ignore them.
>
> > On Jul 24, 2019, at 09:47, wei wei via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> >
> > Hi,
> > I would like to start a new porting of GlobalIsel for RISCV.
> > An initial patch about GlobalIsel infrastructure for RISCV was ready
now:
> > https://reviews.llvm.org/D65219
> > There is another porting patch https://reviews.llvm.org/D41653  posted
> > by Leslie Zhai at the end of 2017. I have checked with Leslie about
> > the status of this patch.He has stopped developing it since some
> > questions need be resolved. There are some discussions about it:
> > http://lists.llvm.org/pipermail/llvm-dev/2018-January/120098.html
> > http://lists.llvm.org/pipermail/llvm-dev/2018-August/125094.html
> > Till now, I think we have a reasonable solution to continue the work,
> > the implementation of GlobalIsel from Mips is a good example, which
> > use target-specific "MipsCCState" and
"MipsCallLowering::MipsHandler"
> > to handle Call/Arguments/Return lowering.
> > For RISCV, there's no "CCAssignFnForCall" or
"CCAssignFnForReturn"
> > functions defined, just like the solution in Mips, a new
> > target-specific "ValueHandler" will be created to support
> > calllowering.
> > I have made some experiment that trying to implement the
"LowerReturn"
> > function, and it can return correctly. The code snippet may be as
> > follows:
> >  ...
> >  CCState CCInfo(F.getCallingConv(), F.isVarArg(), MF, ArgLocs,
F.getContext());
> >  TLI.analyzeOutputArgs(MF, CCInfo, Outs, true, nullptr);
> >  RISCVValueHandler RetHandler(MIRBuilder, MF.getRegInfo(), Ret);
> >  RetHandler.handleArg(ArgLocs, RetInfos);
> >  ...
> > In order to reduce duplicated code as much as possible, and reuse part
> > of code from "TargetLowering" for GlobalIsel, the access
specifiers
> > for some functions in RISCVTargetLowering need be changed, like
> > "analyzeOutputArgs". I have made a separate patch for it:
> > https://reviews.llvm.org/D65218
> > Since Leslie is no longer working on GlobalIsel, I want to continue
> > his work and make some contributions for RISCV target.
> > The code base was changed a lot from 2017, so I restart a new
> > GlobalIsel(RISCV) patch for reviewing, any suggestions or reviewing
> > comments would be appreciated.
> >
> > Thanks,
> > Andrew Wei
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>