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

Ulrich Weigand Ulrich.Weigand at de.ibm.com
Wed Mar 20 12:43:46 PDT 2013


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" ...

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) ...  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'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'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?


Bye,
Ulrich




More information about the llvm-commits mailing list