Weird handling of r+r (pre-inc) addresses

[Hal Finkel]

----- Original Message -----
> From: "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu, "Bill Schmidt" <wschmidt at us.ibm.com>
> Sent: Wednesday, March 20, 2013 2:43:46 PM
> Subject: Weird handling of r+r (pre-inc) addresses
> 
> 
> Hi Hal,
> 
> when working on the pre-inc stuff I kept getting confused which of
> the two
> operands of r+r addresses was the "base" and which the "index".  This
> confusion has only increased with the recent ptr_rc_nor0 addition, so
> I've
> been attempting to figure out what's going on here.   Maybe you have
> some
> thoughts ...
> 
> First of all, memrr is defined as:
> 
> def memrr : Operand<iPTR> {
>   let PrintMethod = "printMemRegReg";
>   let MIOperandInfo = (ops ptr_rc_nor0:$offreg, ptr_rc:$ptrreg);
> }
> 
> so it calls the first operand (which corresponds to RA in the PowerPC
> ISA
> document) the "offset" (i.e. index), and the second operand (which
> corresponds to RB) the "pointer" (i.e. base).   However, RA is the
> register
> that really behaves like the base register in an r+i address:
> register 0 is
> treated specially, and it is the register that is updated in a
> pre-inc
> instruction.  RB on the other hand is just a plain register with no
> special
> treatment.  So it would appear more logical to call the first operand
> "ptrreg" and the second "offreg" ...

Agreed.

> 
> Now at this point this is just naming, so doesn't have any real
> effect.
> But then I got confused further when looking again at the pre-inc
> implementation. Indexed store SD nodes already come with two separate
> "BasePtr" and "Offset" fields, which are presented in this order as
> the
> operands of the pre_store (and related) pattern fragments.  For
> example:
> 
>   def : Pat<(pre_store GPRC:$rS, ptr_rc:$ptrreg, xaddroff:$ptroff),
>             (STWUX GPRC:$rS, xaddroff:$ptroff, ptr_rc:$ptrreg)>;
> 
> Note that "BasePtr" and "Offset" fields of the indexed store node are
> swapped to make up the "offreg" and "ptrreg" fields of the memrr
> operand of
> the underlying STWUX instruction.  This looks reasonable at first
> glance ... but as noted above the "offreg" is really the *base*, and
> not
> the offset, so why is it correct to move BasePtr into it?
> 
> Looking at the selection DAG dumps, however, it turns out that for
> r+r
> indexed load/store nodes, the BasePtr field indeed contains the
> offset, and
> the Offset field the base.  This is because of this code in
> PPCTargetLowering::getPreIndexedAddressParts:
> 
>   if (SelectAddressRegReg(Ptr, Offset, Base, DAG)) {
>     AM = ISD::PRE_INC;
>     return true;
>   }
> 
> Note that the prototype of SelectAddressRegReg is:
> 
> bool PPCTargetLowering::SelectAddressRegReg(SDValue N, SDValue &Base,
>                                             SDValue &Index,
>                                             SelectionDAG &DAG) const
>                                             {
> 
> so the above call fills the base register into "Offset" and the
> offset into
> "Base" ...
> 
> Again, all this swapping probably doesn't have any real effect
> (except for
> being confusing) ... 

And it is confusing... moreover, some of the SelectAddress* functions have Base/Index in one order and some use the opposite ordering.

> except that, going back to the Pat above, we
> now see
> that the *base* value is matched via "xaddroff" at selection time,
> and the
> offset value is matched via "ptr_rc".  The latter is probably in fact
> correct, but the xaddroff is a bit weird.  It seems to work because
> xaddroff just accepts everything except immediates ...
> 
> Wouldn't it be more correct to match the base register via
> ptr_rc_nor0 here
> (just like for r+i instructions)?  Then xaddroff would simply be
> obsolete.

I think that would work.

> 
> 
> I've also tried to revert the swapped base and offset parts in
> indexed
> load/store nodes.  But that runs into additional problems.  If we see
> at
> instruction selection time an address of the form reg + imm where the
> immediate is out of range, this is rejected by SelectAddressRegImm,
> but
> will be *accepted* by SelectAddressRegReg; in this case "Index" will
> be set
> to that immediate.  Instruction selection then loads the immediate
> into a
> register so that it then matches the "ptr_rc" operand.
> 
> However, this works only if that immediate offset is present in the
> "BasePtr" field of the indexed store: were it present in the "Offset"
> field, it would match the iaddroff predicate, this this simply
> accepts
> *any* immediate (with a comment "Because preinc imms have already
> been
> validated, just accept it.").  So this seems to only work with the
> current
> code base because for r+r cases, the "Offset" field actually holds
> the
> base, which usually is already a register without needing to be
> reloaded.
> I'm not sure this is 100% reliable though ...

I think that some other part of the system will actually coerce the immediates into registers if need be.

> 
> I'd try to fix all this along the lines of:
> - remove xaddroff in favor of ptr_rc_nor0 as discussed above
> - rewrite iaddroff to properly check for in-range immediates
> - undo the swapping of BasePtr and Offset for r+r indexed
> loads/stores
> (both when creating and using them)
> - swap the "offreg" and "ptrreg" names of the memrr sub-operands
> 
> Does this make sense, or am I missing some reason why things have to
> be
> this way?

I don't think that you're missing anything. I think that cleaning this up and making it truly self-consistent would be great.

Thanks again,
Hal

> 
> 
> Bye,
> Ulrich
> 
>