[PATCH] D48029: [DAGCombine] Fix alignment for offset loads/stores

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12185-12186
     if (unsigned Align = DAG.InferPtrAlignment(Ptr)) {
-      if (Align > LD->getMemOperand()->getBaseAlignment()) {
+      if (Align > LD->getOriginalAlignment() &&
+          LD->getSrcValueOffset() % Align == 0) {
         SDValue NewLoad = DAG.getExtLoad(
----------------
arsenm wrote:
> dmgreen wrote:
> > arsenm wrote:
> > > dmgreen wrote:
> > > > arsenm wrote:
> > > > > This should just use getAlignment() rather than looking at the two underlying alignments?
> > > > I'm not sure what you mean by two underlying alignments. Do you mean use LD->getAlignment(), not LD->getOriginalAlignment()?
> > > > 
> > > > The Align passed to getExtLoad seems to be treated as a base align, not a base+offset align.
> > > Yes. The original code doesn't make sense to me, if it's looking for an improved alignment it should be looking at alignment the load already has, rather than for some reason looking at the MMO.
> > > 
> > > It's the alignment of the load piece itself. The "base alignment" refers to the alignment of the IR piece, and really nothing in the DAG should be particularly concerned with it.
> > I'm not sure if the load has a alignment itself, unless I'm reading this wrong. The MemSDNode::getAlignment() just calls MMO->getAlignment(), which is MinAlign(getBaseAlignment(), getOffset()).
> > 
> > So I was going with getBaseAlignment as it's a simpler call and Align needs to treated as a BaseAlign (which the mod check ensures is valid). Happy enough to change it.
> Yes, the load alignment is ultimately stored in the MMO. I meant it shouldn't need to be looking at the underlying separate IR pieces and worrying about if it's at an offset from the base. Does this not work if you just change it to getAlignment() for some reason?
The chain of events that happen in the attached test case is we have these:
  t23: ch = store<(store 4 into %ir.helper.20.32)> t0, Constant:i32<0>, t3, undef:i32
  t21: ch = store<(store 4 into %ir.helper.24.32, align 8)> t0, Constant:i32<0>, t29, undef:i32
  t27: i64,ch = load<(load 8 from %ir.helper.20.64, align 4)> t26, t3, undef:i32
The load is a 64bit unaligned load from %helper + 20. The stores are 32bit to %helper+20 and %helper+24. %helper is align 8.

The load is legalised to these 32bit loads:
  t30: i32,ch = load<(load 4 from %ir.helper.20.64)> t26, t3, undef:i32
  t33: i32,ch = load<(load 4 from %ir.helper.20.64 + 4)> t26, t32, undef:i32
The second one, via it being a Frame Index in InferPtrAlignment, is deemed to have an alignment of 8 (which is true for the load, (20+4)%8==0, but not for the base).

So if we use Align in this getExtLoad, it is passes to getLoad, which passes it to MachineFunction::getMachineMemOperand as base_align. So the base_align on the MMO gets set to 8, and we end up with this:
  Combining: t35: i32,ch = load<(load 4 from %ir.helper.20.64 + 4)> t21, t37, undef:i32
  Creating new node: t38: i32,ch = load<(load 4 from %ir.helper.20.64 + 4, align 8)> t0, t37, undef:i32
  Creating new node: t39: ch = TokenFactor t21, t38:1
  Replacing.1 t35: i32,ch = load<(load 4 from %ir.helper.20.64 + 4, align 8)> t21, t37, undef:i32
  With: t38: i32,ch = load<(load 4 from %ir.helper.20.64 + 4, align 8)> t0, t37, undef:i32
The "align 8" that magically appeared on t35 is the base align on %ir.helper.20.64, not the align on %ir.helper.20.64+4.

The actual combine it's reporting here is from the code in DAGCombiner::isAlias (see line 17955) using this base alignment (getOriginalAlignment()) to conclude that the load (t35) and a store (t21 to %ir.helper.24.32) don't alias. So it sets the chain to t0, and the load then gets scheduled before the store.

It seems to me that setting the alignment here, presuming it's a base_align, is the part that's wrong (as opposed to any other part of this chain of events.) As Eli says though, the API here is really confusing, with it's multiple types of alignment, etc.


https://reviews.llvm.org/D48029