Page MenuHomePhabricator

[AArch64][GlobalISel] Relax oneuse restriction for PTR_ADD chain combining to check addressing legality.
ClosedPublic

Authored by aemerson on Jul 8 2021, 9:43 PM.

Details

Summary

Fixing this after @foad reported that the oneuse restriction regressed some code. I couldn't reproduce this without essentially disabling reassociation since that ends up optimizing the test case anyway.

Diff Detail

Event Timeline

aemerson created this revision.Jul 8 2021, 9:43 PM
aemerson requested review of this revision.Jul 8 2021, 9:43 PM

@foad does this fix your internal regression?

@foad does this fix your internal regression?

@sebastian-ne can you try it out please?

does this fix your internal regression?

Yes, thanks a lot.
FWIW, the “internal regression” is D103326, all the test changes there were gone after I rebased it upon D105069.

There is now only a single case, where slightly worse code than is generated compared to before D105069: https://github.com/llvm/llvm-project/blob/c282d55a38577e076b48cd7a8113e5eb0a2039cd/llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.ll#L4151-L4164
isLegalAddressingMode correctly returns false and the dead loads from the <64 x i32> load (which are multiple users of the G_PTR_ADD) are only removed later, after the G_PTR_ADD has long been converted to pseudo instructions. I guess there’s not much we can do about that.

Thinking more about it, maybe it makes sense to only skip the combine if isLegalAddressingMode(MaybeImm2Val->Value) && !isLegalAddressingMode(CombinedImm)?
(That should also fix the case I mentioned in the last comment.)

gentle reverse ping

sebastian-ne commandeered this revision.Jul 29 2021, 5:51 AM
sebastian-ne added a reviewer: aemerson.

Add check that addressing mode was legal before, but is not legal anymore after adding the offsets.

aemerson added inline comments.Jul 29 2021, 11:09 AM
llvm/test/CodeGen/AArch64/GlobalISel/combine-ptradd-reassociation.mir
26–27

I think this test is actually wrong, not sure how it slipped through. To properly test the reassociation behavior we should use a legal offset. Can you replace the 4777 with 1600 and the 6 with 3?

176–177

Same here.

aemerson added inline comments.Jul 29 2021, 11:25 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1731–1734

Also, we now have the GLoadStore wrapper class that can be used here with a dyn_cast.

1735

and this can then be replaced with LdStMI.getReg(0)

aemerson commandeered this revision.Aug 10 2021, 4:07 PM
aemerson edited reviewers, added: sebastian-ne; removed: aemerson.

Hope you don't mind me taking back this revision to address my comments and commit it.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 10 2021, 4:41 PM
This revision was automatically updated to reflect the committed changes.