Page MenuHomePhabricator

[AlignmentFromAssumptions] Fix a SCEV assertion resulting from address space differences.
ClosedPublic

Authored by DiamondLovesYou on Mar 2 2020, 10:15 AM.

Details

Summary

On targets with different pointer sizes, -alignment-from-assumptions could attempt to create SCEV expressions which use different effective SCEV types. The provided test illustrates the issue.

In getNewAlignment, AASCEV would be the (only) alloca, which would have an effective SCEV type of i32. But PtrSCEV, the GEP in this case, due to being in the flat/default address space, will have an effective SCEV of i64.

This patch resolves the issue by truncating PtrSCEV to AASCEV's effective type.

Diff Detail

Event Timeline

Fix CI issue ("no CHECK line").

I think this is generally fine, except the test case changes noted below.

FWIW, this pass will be deleted (hopefully soon).

llvm/test/Transforms/AlignmentFromAssumptions/amdgpu-crash.ll
4

If you want to test for "non-crash", remove the FileCheck. However, I think you want to test that the generated expression is actually as expected instead.

48

Please remove all these attributes and metadata notes, etc., none of it should be needed.

Is this still active?

jdoerfert requested changes to this revision.Thu, Mar 19, 9:41 PM
This revision now requires changes to proceed.Thu, Mar 19, 9:41 PM
DiamondLovesYou updated this revision to Diff 252114.EditedMon, Mar 23, 12:04 PM

Clean up the test.

DiamondLovesYou marked 3 inline comments as done.Mon, Mar 23, 12:20 PM

Sorry for the delay; yes this is still active.

llvm/test/Transforms/AlignmentFromAssumptions/amdgpu-crash.ll
4

If you want to test for "non-crash", remove the FileCheck.

Ah, should have known, thanks.

I think you want to test that the generated expression is actually as expected instead.

There's nothing to test (aside from not crashing): -alignment-from-assumptions doesn't actually change anything in this test, even if I remove the alignment on the store or up the alignment from the assumption and alloca.

This revision is now accepted and ready to land.Mon, Mar 23, 5:45 PM
This revision was automatically updated to reflect the committed changes.
DiamondLovesYou marked an inline comment as done.