This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Fix combine crash because LI is not set in prelegalizer
ClosedPublic

Authored by Petar.Avramovic on Nov 2 2022, 11:10 AM.

Details

Summary

Caused by legacy min/max combines (select + cmp) asking for legalizer info in prelegalizer (D135047 added combine to all_combines).
Combine still does not work for AMDGPU since destination opcode is custom, not legal. Similar combine works on DAG since it asks for legal or custom.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 11:10 AM
Petar.Avramovic requested review of this revision.Nov 2 2022, 11:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 11:10 AM

To my understanding prelegalizer now also requires legalizer info?

llvm/test/CodeGen/AMDGPU/cttz_zero_undef.ll
743

Change is caused by zextload being combined pre-legalizer

arsenm accepted this revision.Nov 2 2022, 12:33 PM
arsenm added inline comments.
llvm/test/CodeGen/AMDGPU/cttz_zero_undef.ll
1619

This is a regression without getting rid of one of these ands

This revision is now accepted and ready to land.Nov 2 2022, 12:33 PM

To my understanding prelegalizer now also requires legalizer info?

Yes, that's what the IsPreLegalize argument is for since we need LI to be valid.

Correction:

Change is caused by zextload being combined pre-legalizer

Change is caused by zextload NOT being combined pre-legalizer

s16 zextload is not legal and with LI availabe in prelegalizer combiner does not perform zextload combine.
Regression is caused by additional zext.

Using if(!isPreLegalize()) as an old equivalent of if(LI) to avoid regression but I am not sure if it is correct behavior with the prelegalizer now also checking for legality. Overall I would prefer to use isPreLegalize() in this patch and fix LI uses in prelegalizer in another patch if needed.

If we don't make zextload in prelegalizer (and let legalizer deal with it) it becomes very difficult to combine, we are left with zext + lowered any_extending_load. It would require known bits analysis that would try to treat that anyextending load as zero or sign extending load (whichever is better) and perform combine and change type of load. This is not visible in the asm since we select any_extending load in the same way as zero_extending load.

Petar.Avramovic requested review of this revision.Nov 3 2022, 8:45 AM
arsenm accepted this revision.Nov 7 2022, 10:30 AM

Prelegalize combining to zextload pre-legalize even if illegal is useful

This revision is now accepted and ready to land.Nov 7 2022, 10:30 AM