Page MenuHomePhabricator

[AArch64][SVEIntrinsicOpts] Fold sve_convert_from_svbool(zero) to zero
ClosedPublic

Authored by joechrisellis on Apr 14 2021, 4:13 AM.

Diff Detail

Event Timeline

joechrisellis created this revision.Apr 14 2021, 4:13 AM
joechrisellis requested review of this revision.Apr 14 2021, 4:13 AM
Herald added a project: Restricted Project. ยท View Herald TranscriptApr 14 2021, 4:13 AM
Herald added a subscriber: llvm-commits. ยท View Herald Transcript

Why is this kind of thing not done in instcombine? Or even constant folding if that is what this is really doing.

Hi @dmgreen! This is SVE-specific, and SVEIntrinsicOpts.cpp is where such transformations are typically placed (at least for now). I did a quick grep and it seems SVE intrinsics don't currently have much of a presence in generic passes like instcombine/constant folding, perhaps because some SVE optimisations are more complex than others and I guess it makes sense to keep them all in the same place.

X86 also has llvm/lib/Target/X86/X86InstCombineIntrinsic.cpp which houses instcombine-like optimisations for X86. ๐Ÿ™‚

Hi @dmgreen! This is SVE-specific, and SVEIntrinsicOpts.cpp is where such transformations are typically placed (at least for now). I did a quick grep and it seems SVE intrinsics don't currently have much of a presence in generic passes like instcombine/constant folding, perhaps because some SVE optimisations are more complex than others and I guess it makes sense to keep them all in the same place.

X86 also has llvm/lib/Target/X86/X86InstCombineIntrinsic.cpp which houses instcombine-like optimisations for X86. ๐Ÿ™‚

Hmm. "Because we've been doing it wrong" isn't a great reason to _keep_ doing it wrong :-)

This is very similar to https://reviews.llvm.org/rGbecaa6803ab532d15506829f0551a5fa49c39d7e, but special cased for zeroinit vectors. I do see this code at the start of that function, but it seems like it could still be made to work, and presumably in the long run some of the other constant folding there might be useful (like masked loads with zero masks).

// Do not iterate on scalable vector. The number of elements is unknown at
// compile-time.
if (isa<ScalableVectorType>(VTy))
  return nullptr;

More generally, any backend can use instCombineIntrinsic to add intrinsic folds to instcombine. I can appreciate that not everything that SVECombines is doing will fit well there, but the ones that do should be put there if possible. The steady state combining it does will be worthwhile, as well as it being run multiple times through the pipeline. Plus, you know, not re-inventing the wheel.

joechrisellis planned changes to this revision.Apr 15 2021, 6:39 AM

@dmgreen -- I hear you, makes sense to me. There might be some functionality already in SVEIntrinsicOpts.cpp that we can pull out, but I guess that can wait for later. In any case I'll move the functionality here to the other passes to reflect your suggestions.

Appreciate your comments! ๐Ÿ˜„

Address review comments.

  • @dmgreen: move transformation to constant folding.
dmgreen accepted this revision.Fri, Apr 16, 5:57 AM

Nice one, thanks. This Looks good to me.

llvm/lib/Analysis/ConstantFolding.cpp
2983

Doesn't need a break after the return (unless it's quieting a warning, but I think most compilers treat this sensibly).

llvm/test/Transforms/InstSimplify/ConstProp/AArch64/aarch64-sve-convert-from-svbool.ll
2

If this folder is new it may need (or at least be best) to have a lit.local.cfg to only run it when AArch64 is a registered target.

This revision is now accepted and ready to land.Fri, Apr 16, 5:57 AM
joechrisellis marked 2 inline comments as done.

Address review comments.

  • @dmgreen:
    • Add lit.local.cfg file to AArch64 directory.
    • Fix minor editing error.
paulwalker-arm accepted this revision.Mon, Apr 19, 2:11 AM
This revision was landed with ongoing or failed builds.Tue, Apr 20, 3:03 AM
This revision was automatically updated to reflect the committed changes.