Page MenuHomePhabricator

[CodeGen] Support folds of not(cmp(cc, ...)) -> cmp(!cc, ...) for scalable vectors

Authored by david-arm on Jan 13 2022, 4:11 AM.



I have updated TargetLowering::isConstTrueVal to also consider
SPLAT_VECTOR nodes with constant integer operands. This allows the
optimisation to also work for targets that support scalable vectors.

Diff Detail

Event Timeline

david-arm created this revision.Jan 13 2022, 4:11 AM
david-arm requested review of this revision.Jan 13 2022, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 4:11 AM

Nice, I'd missed that one. I was going to ask about SPLAT_VECTOR_PARTS but I'm not sure we could get a test for that, seeing as I think it's only generated for 64-bit integer splats on RV32.


I think using --check-prefixes=CHECK,RV32 is fairly common in our RVV testing and looks like it'd help remove all duplicate checks.

david-arm updated this revision to Diff 399637.Jan 13 2022, 5:01 AM
  • Used better check prefixes for RISCV tests.
david-arm marked an inline comment as done.Jan 13 2022, 5:01 AM
david-arm added inline comments.

Good suggestion, thanks!

frasercrmck added inline comments.Jan 13 2022, 8:02 AM

Oh but of course if there's no use of the RV32 or RV64 checks then lit will fail, sorry about that: I always forget. Sounds like you can just use FileCheck %s

david-arm updated this revision to Diff 400485.Jan 17 2022, 3:28 AM
david-arm marked an inline comment as done.
  • Removed unnecessary CHECK lines.
david-arm marked an inline comment as done.Jan 17 2022, 3:29 AM
frasercrmck added inline comments.Jan 17 2022, 3:30 AM

I think we have to support the truncating case as with BUILD_VECTOR above, don't we?

david-arm added inline comments.Jan 17 2022, 3:45 AM

Hmm, I looked at the definition of SPLAT_VECTOR and you're right - the operand is allowed to be wider than the vector element type of the result. I hadn't realised that :(. The only problem is that I have absolutely no idea how to write a test case for this using IR! I'm not sure if there is a way to create a splat (using SPLAT_VECTOR) in IR that is truncating. Unless you know of any RISCV examples where this would happen?

david-arm updated this revision to Diff 400497.Jan 17 2022, 5:10 AM
  • Add support for truncating splats.
frasercrmck added inline comments.Jan 18 2022, 8:07 AM

Hmm, it would definitely happen during legalization as we only have either i32 (RV32) or i64 (RV64) as legal integer types, so most splats, really, like <vscale x 1 x i8> with a constant integer. But you'd need to be calling this during/after legalization. So no I'm not sure it's testable, per se. What you've got looks right, though?

sdesmalen added inline comments.Jan 20 2022, 7:26 AM

Instead of checking for SPLAT_VECTOR explicitly, how about:

if (auto *CN = dyn_cast<ConstantSDNode>(N)) {
  CVal = CN->getAPIntValue();
} else if (isConstantSplatVector(N, CVal)) {
  unsigned EltWidth = N->getValueType(0).getScalarSizeInBits();
  if (EltWidth < CVal.getBitwidth())
    CVal = CVal.trunc(EltWidth);
} else
  return false;
david-arm added inline comments.Jan 21 2022, 1:42 AM

Sadly this doesn't work because isConstantSplatVector demands the element width of the result be the same as the element width in the BuildVectorSDNode, which means we end up with lots of failing tests. It's just a minor point, but isConstantSplatVector also permits FP splats, so we'd have to restrict it to integer too.

sdesmalen added inline comments.Jan 27 2022, 9:36 AM

In that case, maybe you can use

ConstantSDNode* llvm::isConstOrConstSplat(SDValue N, bool AllowUndefs, bool AllowTruncation)

which allows you to specify whether truncation is allowed?

It's bonkers how many of these functions exist...

david-arm updated this revision to Diff 403963.Jan 28 2022, 4:17 AM
  • Updated isConsTrueVal to use isConstOrConstSplat.
david-arm marked 4 inline comments as done.Jan 28 2022, 4:17 AM

Thanks for the change, I think this is an improvement since we can now reuse some existing code for recognising the splat!


Can you update isConstFalseVal as well to only take an SDValue as part of this patch?

david-arm updated this revision to Diff 403983.Jan 28 2022, 5:33 AM
david-arm marked an inline comment as done.
sdesmalen accepted this revision.Jan 28 2022, 5:53 AM

Thanks @david-arm, LGTM!

This revision is now accepted and ready to land.Jan 28 2022, 5:53 AM
This revision was landed with ongoing or failed builds.Feb 1 2022, 1:50 AM
This revision was automatically updated to reflect the committed changes.