Page MenuHomePhabricator

[Analysis] add isSplatValue() for vectors in IR
ClosedPublic

Authored by spatel on Jun 11 2019, 7:09 AM.

Details

Summary

We have the related getSplatValue() already in IR (see code just above the proposed addition). But sometimes we only need to know that the value is a splat rather than capture the splatted scalar value. Also, we have an isSplatValue() function already in SDAG.

Motivation - recent bugs that would potentially benefit from improved splat analysis in IR:
https://bugs.llvm.org/show_bug.cgi?id=37428
https://bugs.llvm.org/show_bug.cgi?id=42174

Diff Detail

Event Timeline

spatel created this revision.Jun 11 2019, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 7:09 AM

Seems ok to me

llvm/lib/Analysis/VectorUtils.cpp
350

also fneg
also some intrinsics - overflow, saturating, ...

RKSimon added inline comments.Jun 11 2019, 7:35 AM
llvm/include/llvm/Analysis/VectorUtils.h
82

Make it clear that this is more powerful than getSplatValue ?

spatel updated this revision to Diff 204075.Jun 11 2019, 8:01 AM

Patch updated:

  1. Add text to header comment to indicate that this may be more powerful than getSplatValue().
  2. Add TODO comment about recursing through several other types of operands.
shawnl added a subscriber: shawnl.Jun 11 2019, 9:21 AM

Looks fine

llvm/lib/Analysis/VectorUtils.cpp
354

We do not currently normalize vector selects to select.

see https://bugs.llvm.org/show_bug.cgi?id=41777

Should we? (as long as it doesn't require defining undefs?).

spatel marked an inline comment as done.Jun 11 2019, 10:03 AM

We have 2 text comment acknowledgements ("Seems fine", "Looks ok") - thanks for the prompt reviews!...can someone make it official by toggling the Phab state to 'Approved'? :)

llvm/lib/Analysis/VectorUtils.cpp
354

Let me know if I'm not getting the question right, but we have to be careful here to distinguish 'vector select' from 'bitwise select' (xxsel or vsel in PPC).

In IR, vector select is based on vector elements rather than bits:
http://llvm.org/docs/LangRef.html#select-instruction
(so it's more like the x86 'blend' instructions).

So to normalize to the IR vector select, we have to create something like this from a bitwise logic sequence

or (and x, cond), (and y, (not cond)) -->
%bitwise_cond = bitcast <4 x i32> %cond to <128 x i1>
%trueval = bitcast <4 x i32> %x to <128 x i1>
%falseval = bitcast <4 x i32> %y to <128 x i1>
%select = select <128 x i1> %bitwise_cond, <128 x i1> %trueval, <128 x i1> %falseval
%result = bitcast <128 x i1> %select to <4 x i32>
This revision is now accepted and ready to land.Jun 11 2019, 10:07 AM
nikic added inline comments.Jun 11 2019, 11:27 AM
llvm/lib/Analysis/VectorUtils.cpp
333

This seems potentially problematic, as there's no guarantee that all (independent) undef values will be chosen to be the same value down the line.

I don't know if this is a problem in the context where this is intended to be used though. Is isSplatValue() going to be used heuristically (for profitability) or is it necessary for transformation correctness?

lebedev.ri marked an inline comment as done.Jun 11 2019, 11:42 AM
lebedev.ri added inline comments.
llvm/lib/Analysis/VectorUtils.cpp
333

This is correct and is in line with other cases like this.
undef here basically means "this can have any value, we don't care", the uses won't care about that lane.
We can define undef to have some value, so we could just fill the undef lines with the splat value, and this get back to the original situation of a splat vector.

spatel marked an inline comment as done.Jun 11 2019, 11:50 AM
spatel added inline comments.
llvm/lib/Analysis/VectorUtils.cpp
333

We have both use cases represented in the motivating bugs:

  1. PR37428 is a profitability transform that could potentially happen in CGP.
  2. PR42174 is a canonicalization transform that could potentially happen in instcombine.

And given that we already have users of getSplatValue(), and it would be difficult to determine the safety on those, I was thinking that we'd add an optional 'AllowUndef' parameter to enable/disable that possibility. This would be similar to the SDAG 'wrapper' variant of this call:

/// Test whether \p V has a splatted value.
bool isSplatValue(SDValue V, bool AllowUndefs = false);

I'm not sure yet if we need/want to go further like the SDAG implementation and actually track the undef/demanded elements:

/// Test whether \p V has a splatted value for all the demanded elements.
///
/// On success \p UndefElts will indicate the elements that have UNDEF
/// values instead of the splat value, this is only guaranteed to be correct
/// for \p DemandedElts.
///
/// NOTE: The function will return true for a demanded splat of UNDEF values.
bool isSplatValue(SDValue V, const APInt &DemandedElts, APInt &UndefElts);
This revision was automatically updated to reflect the committed changes.