[PATCH] D37164: [ARM] Fix bug in ARMLoadStoreOptimizer when kill flags are missing.

[Geoff Berry via Phabricator via llvm-commits]

gberry added a comment.

In https://reviews.llvm.org/D37164#854223, @MatzeB wrote:

> In https://reviews.llvm.org/D37164#854194, @MatzeB wrote:
>
> > In https://reviews.llvm.org/D37164#854053, @gberry wrote:
> >
> > > In https://reviews.llvm.org/D37164#853040, @efriedma wrote:
> > >
> > > > If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?
> > >
> > >
> > > That's what it looks like to me: an attempt to reduce the calls to regsOverlap().  Or at least that would make sense for 'BaseKill'.  Checking 'OffKill' I don't get unless 'Base' and 'Off' are somehow known to be related?
> >
> >
> > Indeed checking for the kill flags is odd and removing the checks looks good to me. However there may be something about the offset register: Won't we have the same problem when the first load overwrites the offset register? So maybe we need to check for an overlap with offsetreg as well (and have a bigger problem if base+offset get overridden?)
>
>
> This whole OffKill/OffUndef handling is confusing. We only ever query the kill and undef flag and don't even bother to query the register itself. We then pass the kill/undef values to `InsertLDR_STR()` which doesn't even use the parameter.
>
> Maybe this is because t2LDRDi8 has an immediate offset and hence no offset register is used, while LDRD/STRD always have consecutive even/odd registers so we should always hit the `OddRegNum > EvenRegNum && OffImm == 0` case earlier in the function in the cases where an offset register is present.
>
> At least smells to me like OffUndef/OffKill uses could be replaced with `assert(!isT2 || MI->getOperand(3).getReg() == 0);`


Yeah, I cam to the same conclusion.  I was unable to trigger this assert in any lit or test-suite test, so perhaps we are never using a register offset for this opcode?


https://reviews.llvm.org/D37164