[PATCH] D61321: [globalisel] Allow SrcOp to convert an APInt and render it as an immediate operand (MO.isImm() == true)

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In D61321#1611626 <https://reviews.llvm.org/D61321#1611626>, @arsenm wrote:

> In D61321#1611613 <https://reviews.llvm.org/D61321#1611613>, @dsanders wrote:
>
> > In D61321#1611610 <https://reviews.llvm.org/D61321#1611610>, @dsanders wrote:
> >
> > > I'm currently rebasing this patch switching APInt to uint64_t and one thing I'm a bit worried about is the case where the implicit conversion from Register to unsigned causes registers to be passed where immediates are expected. At the moment our plan guards against the case where Register is correctly used but not against trivial misuse like:
> > >
> > >   unsigned Reg = Register();
> > >   SrcOp(Reg)
> > >
> > >
> > > and with the current unsigned constructor, this:
> > >
> > >   SrcOp(1)
> > >
> > >
> > > will be treated as a register.
> > >
> > > I had a look into the feasibility of eliminating the implicit conversions and that's way too big a task at the moment to block this on fixing that (I'll post some patches that propagate Register a bit more tomorrow, they were a side-effect of looking into it). I have a potential alternative though. We can add some deleted constructors like in https://godbolt.org/z/0zQOJX. The downside of this is plain constants like `0` won't work, you need to use `0ul`. Does that limitation sound ok if it makes mixing up registers and immediates harder?
> >
> >
> > Of course, the moment I post I notice a case I'd missed. AArch64::WZR is an unsigned, we'd have to pass it as `Register(AArch64::WZR)` to avoid hitting the deleted constructor
>
>
> This is already an issue I wasn’t able to come up with a good solution for independent of adding the new imm SrcOp


The main thing I need to establish to update and land a version of this patch is what our preference is. Do we prefer?:

1. having immediates wrapped in APInt and registers being unsigned/Register. This is more compatible with the current code.
2. immediates being APInt/uint64_t/int64_t, immediate constants being 0ul, registers being wrapped in Register, and unsigned being invalid. Later when the implicit Register->unsigned conversion is deleted re-allow unsigned as immediates
3. something else

1. is less invasive to the current code and is much easier to land but 2. better aligns with our long term thoughts on eliminating unsigned for registers. My opinion depends on difficulty with 2 being my first choice and 1 being my fallback if changing unsigned->Register significantly exceeds my expectations in terms of how much porting work to the existing code is needed.

What are your preferences?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61321/new/

https://reviews.llvm.org/D61321