This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Add check for BUILD_VECTOR in isKnownNeverNaN
ClosedPublic

Authored by Petar.Avramovic on Sep 30 2020, 7:40 AM.

Details

Summary

This includes handling of constants with vector type in isKnownNeverNaN.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2020, 7:40 AM
Petar.Avramovic requested review of this revision.Sep 30 2020, 7:40 AM
foad added inline comments.Sep 30 2020, 7:56 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4685–4686

It looks like you're adding new functionality, so update the comment in SelectionDAG.h to explain that this function now handles vectors and what the behaviour is?

4784

for (const SDValue &Opnd : Op->ops()) ?

Petar.Avramovic edited the summary of this revision. (Show Details)

Patch also affects constants with vector type. Effectively, during legalization, fcanonicalize will not be inserted when input was vector constant.

define amdgpu_kernel void @maxnum_v2f16_imm_b(<2 x half> addrspace(1)* %r, <2 x half> addrspace(1)* %a) {
entry:
  %a.val = load <2 x half>, <2 x half> addrspace(1)* %a
  %add = fadd <2 x half> %a.val, <half 9.0, half 8.0>
  %r.val = call <2 x half> @llvm.maxnum.v2f16(<2 x half> %add, <2 x half> <half 4.0, half 3.0>) ;<half 4.0, half 3.0> will not get fcanonicalized during legalization after this patch
  store <2 x half> %r.val, <2 x half> addrspace(1)* %r
  ret void
}

This won't be visible in end result since there is fcanonicalize + build_vector combine in SITargetLowering::performFCanonicalizeCombine. Test changes were for non-constant build_vectors.
Add comment to indicate vector constants are handled bellow.

foad added a comment.Oct 5 2020, 2:14 AM

@ecnelises did you really mean to add yourself as a "blocking reviewer" rather than a normal reviewer?

foad accepted this revision.Oct 5 2020, 2:25 AM

LGTM apart from the nitpick about comments.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4685–4686

s/bellow/below/, or you could just delete this comment.

I meant something in SelectionDAG.h like:

/// Test whether the given SDValue **(or all elements of it, if it is a vector)** are known to never be NaN. If \p SNaN is
/// true, returns if \p Op is known to never be a signaling NaN (it may still
/// be a qNaN).
bool isKnownNeverNaN(SDValue Op, bool SNaN = false, unsigned Depth = 0) const;

Just to say explicitly what the expected behaviour is for a vector.

@ecnelises did you really mean to add yourself as a "blocking reviewer" rather than a normal reviewer?

Oh, sorry! I meant to add myself as subscriber..

ecnelises resigned from this revision.Oct 16 2020, 12:01 AM
This revision is now accepted and ready to land.Oct 16 2020, 12:01 AM
arsenm accepted this revision.Sep 28 2022, 2:24 PM

LGTM

Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 2:24 PM
Herald added a subscriber: kosarev. · View Herald Transcript

Reverse ping