Page MenuHomePhabricator

[AArch64][GlobalISel] Add post-legalize combine for sext(trunc(sextload)) -> trunc/copy
ClosedPublic

Authored by aemerson on Jun 17 2020, 12:00 AM.

Diff Detail

Event Timeline

aemerson created this revision.Jun 17 2020, 12:00 AM

This doesn't seem necessary for correctness? I think we do need some optimizing combines in the legalizer itself, but I assumed we would start putting those in a distinct place

This doesn't seem necessary for correctness? I think we do need some optimizing combines in the legalizer itself, but I assumed we would start putting those in a distinct place

True, I suppose we need to be stricter about these combines. I'll move this into the post legalizer combiner.

aemerson updated this revision to Diff 271520.Jun 17 2020, 4:32 PM
aemerson retitled this revision from [AArch64][GlobalISel] Add artifact combine for sext(trunc(sextload)) -> trunc/copy to [AArch64][GlobalISel] Add post-legalize combine for sext(trunc(sextload)) -> trunc/copy.

Move the combine to post legalizer, now matching what ends up coming out of the legalizer for AArch64 which is a G_SEXT_INREG.

arsenm added inline comments.Jun 17 2020, 5:12 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
588

This is just a special case of checking computeNumSignBits? Do we recognize the number of sign bits in a sextload yet?

aemerson marked an inline comment as done.Jun 19 2020, 12:51 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
588

We don't, but using computeNumSignBits doesn't tell us how to find the original def in the chain that has redundantly been sign extended. E.g. in this case we won't know how to look through the truncate.

aemerson updated this revision to Diff 272264.Jun 20 2020, 10:17 AM

Use computeNumSignBits() since we can tolerate extra copies with SEXT_INREG.

arsenm added inline comments.Jun 22 2020, 1:35 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
586

This should be >=?

592

I don't think introducing a new AnyExt would be right?

aemerson updated this revision to Diff 274927.Jul 1 2020, 2:49 PM

Address comments. Use copy instead of buildAnyExtOrTrunc.

arsenm added inline comments.Jul 10 2020, 3:10 PM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
447

I think this needs to be careful about a vector sextload

aemerson updated this revision to Diff 277495.Mon, Jul 13, 11:07 AM

Don't try to handle vector SEXTLOAD in KB.

arsenm accepted this revision.Mon, Jul 13, 12:08 PM

LGTM with nits for future vector handling

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
584

getScalarSizeInBits

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
448–449

Should add TODO to handle vectors

This revision is now accepted and ready to land.Mon, Jul 13, 12:08 PM
This revision was automatically updated to reflect the committed changes.

I reverted this because it turns out that computeNumSignBits() doesn't actually do what we need here. Its definition of sign bits are any top-most bits which are known to be identical. Because of that it matches G_ZEXTLOAD with the known zero bits at the top returns as "sign bits". This behavior seems to be consistent with the SDAG KB implementation too.

I committed the original version of this patch in 3b10e42ba1a3ff97346bc51f13195ed9595800f4, with the vector bail out added. Let me know if I should change it post commit.