[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:
> > > 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.


https://reviews.llvm.org/D48029