This is an archive of the discontinued LLVM Phabricator instance.

[IRBuilder] Use PoisonValue in CreateMasked*
ClosedPublic

Authored by vitalybuka on Sep 15 2022, 12:49 PM.

Diff Detail

Event Timeline

vitalybuka created this revision.Sep 15 2022, 12:49 PM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
vitalybuka requested review of this revision.Sep 15 2022, 12:49 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
vitalybuka edited the summary of this revision. (Show Details)Sep 15 2022, 12:55 PM

Adding some vector folks to review the change of masked out lanes from undef to poison.

Langref doesn't specify a version without a pass-through argument, so this is up to the users of these functions. We need review for:

  • SLP vectorizer
  • Loop vectorizer
  • Codegen's expand vp

Thank you!

I reviewed all users:

  • SLP Vectorizer: the mask is all ones, so passthru is irrelevant
  • Loop vectorizer: masked out lanes aren't used. In fact, masked loads are already using poison for passthru.
  • X86 expand intrinsic: uses custom zero passthru.
  • Codegen expand vp load/gather: there's an issue here as langref says passthru is undef.

So, vector folks, is it ok to change @llvm.vp.load/gather's passthru to poison instead of undef?

I reviewed all users:

  • SLP Vectorizer: the mask is all ones, so passthru is irrelevant
  • Loop vectorizer: masked out lanes aren't used. In fact, masked loads are already using poison for passthru.
  • X86 expand intrinsic: uses custom zero passthru.
  • Codegen expand vp load/gather: there's an issue here as langref says passthru is undef.

So, vector folks, is it ok to change @llvm.vp.load/gather's passthru to poison instead of undef?

Yes and we should do this for all VP intrinsics. This also means we need to change the wording in the Semantics sections, eg https://llvm.org/docs/LangRef.html#id894 .

nlopes accepted this revision.Sep 19 2022, 2:19 AM

I reviewed all users:

  • SLP Vectorizer: the mask is all ones, so passthru is irrelevant
  • Loop vectorizer: masked out lanes aren't used. In fact, masked loads are already using poison for passthru.
  • X86 expand intrinsic: uses custom zero passthru.
  • Codegen expand vp load/gather: there's an issue here as langref says passthru is undef.

So, vector folks, is it ok to change @llvm.vp.load/gather's passthru to poison instead of undef?

Yes and we should do this for all VP intrinsics. This also means we need to change the wording in the Semantics sections, eg https://llvm.org/docs/LangRef.html#id894 .

Thanks for the review, Simon!
I've implemented the change to LangRef here: https://github.com/llvm/llvm-project/commit/32b1b06b7081bd722750c6f3d528336f3f7ed34b

This patch can now proceed. Thank you!

This revision is now accepted and ready to land.Sep 19 2022, 2:19 AM
This revision was landed with ongoing or failed builds.Sep 19 2022, 11:02 AM
This revision was automatically updated to reflect the committed changes.