Page MenuHomePhabricator

[IR] masked gather/scatter alignment should be set
ClosedPublic

Authored by gchatelet on Jan 22 2020, 5:36 AM.

Details

Summary

masked_load and masked_store instructions require the alignment to be specified and a power of two. It seems to me that this requirement applies to masked_gather and masked_scatter as well.

Diff Detail

Event Timeline

gchatelet created this revision.Jan 22 2020, 5:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 5:36 AM
delena added inline comments.Jan 23 2020, 1:00 AM
llvm/test/CodeGen/X86/avx2-masked-gather.ll
54 ↗(On Diff #239553)

I think 0 is also good. You can put alignment 0 in load/store.

gchatelet marked 2 inline comments as done.Jan 23 2020, 1:38 AM
gchatelet added inline comments.
llvm/test/CodeGen/X86/avx2-masked-gather.ll
54 ↗(On Diff #239553)

It seems like it's not the case for masked load / store
unittest
godbolt

Alignment needs to be a non 0 power of two.

Any reasons why we would want it to be 0?

gchatelet marked 2 inline comments as done.Jan 23 2020, 1:49 AM
gchatelet added inline comments.
llvm/test/CodeGen/X86/avx2-masked-gather.ll
54 ↗(On Diff #239553)

Looking at the source code it seems like masked_gather depends on an aligned_load which should have a non zero alignment?

delena added a reviewer: Ayal.Jan 23 2020, 2:04 AM

CreateAlignedLoad allows align 0, as far as I see in the code.
We just assume that masked.gather is created from scalar load by vectorizer. Vectorizer just copies alignment from scalar load to masked load/gather and the requirement to alignment should match load alignment.

CreateAlignedLoad allows align 0, as far as I see in the code.
We just assume that masked.gather is created from scalar load by vectorizer. Vectorizer just copies alignment from scalar load to masked load/gather and the requirement to alignment should match load alignment.

I see, then you're write that 0 should be allowed.
I'm trying to tighten the alignment type throughout LLVM so I always try to have the most restrictive type.
I'll change the Patch accordingly.

gchatelet updated this revision to Diff 239831.Jan 23 2020, 2:46 AM
  • Address comments
gchatelet updated this revision to Diff 239835.Jan 23 2020, 2:51 AM
  • Fixing error message

Any other comments on you side @delena ?

delena accepted this revision.Jan 25 2020, 12:20 PM
This revision is now accepted and ready to land.Jan 25 2020, 12:20 PM
This revision was automatically updated to reflect the committed changes.