This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Always use TargetConstant for FP_ROUND ISD Nodes
ClosedPublic

Authored by DavidTruby on Jul 22 2022, 9:37 AM.

Details

Summary

This patch ensures consistency in the construction of FP_ROUND nodes
such that they always use ISD::TargetConstant instead of ISD::Constant.

This additionally fixes a bug in the AArch64 SVE backend where patterns
were matching against TargetConstant nodes and sometimes failing when
passed a Constant node.

Diff Detail

Event Timeline

DavidTruby created this revision.Jul 22 2022, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 9:37 AM
DavidTruby requested review of this revision.Jul 22 2022, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 9:37 AM

Correct call to getTargetConstant in PPC backend

Why shouldn't it always be a TargetConstant? TargetConstant is what we use for ImmArg on intrinsics. This feels similar since we expect it to always be a constant.

I think this should consistently be an i32 TargetConstant. There’s no reason to change it based on the pointer type or target

TargetConstant's type is not checked for legality, right? In this case it could be i1 as well.

Essentially I went with Constant because there were significantly more cases where getIntPtrConstant was used to construct the argument here than getTargetConstant. I'd be pretty happy with either as long as its consistent though! I suppose if it works correctly an i1 TargetConstant would probably make the most logical sense.

craig.topper added inline comments.Jul 22 2022, 3:14 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
21213

Is there any real reason that FP_ROUND_MERGE_PASSTHRU even needs this flag? Or is just because LowerToPredicatedOp doesn't know to drop it?

barannikov88 added a comment.EditedJul 22 2022, 3:19 PM

Essentially I went with Constant because there were significantly more cases where getIntPtrConstant was used to construct the argument here than getTargetConstant. I'd be pretty happy with either as long as its consistent though! I suppose if it works correctly an i1 TargetConstant would probably make the most logical sense.

It may be worth extracting creation of FP_ROUND / STRICT_FP_ROUND into dedicated method of SelectionDAG (getFPExtendOrRound is an example, and it, by the way, uses getIntPtrConstant). This would localize creation of the constant operand.

Matt added a subscriber: Matt.Jul 23 2022, 7:54 PM
llvm/test/CodeGen/AArch64/sve-fcopysign.ll
257

Need to rerun update test checks?

paulwalker-arm added inline comments.Jul 25 2022, 4:39 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
21213

This stems for a rule we have whereby when trivially extending a common ISD nodes to an AArch64 predicated version, we honour the original's definition. This just means nobody should be surprised by a node with the sameish name but a different set/order of operands (i.e. the predication is purely additive).

Switch to using TargetConstant instead of Constant.

When I tried to switch to a specific TargetConstant type (e.g. MVT::i32 or
MVT::i1) this caused a lot of churn in a number of backends. By extension,
switching to a getFPRound function caused the same. I think if we want to go
down that road it should be done in a separate patch.

DavidTruby retitled this revision from [llvm] Always use Constant for FP_ROUND ISD Nodes to [llvm] Always use TargetConstant for FP_ROUND ISD Nodes.Jul 26 2022, 7:40 AM
DavidTruby edited the summary of this revision. (Show Details)
craig.topper added inline comments.Jul 26 2022, 9:04 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
15766–15768

lines should wrap before 80 columns. Use clang-format.

clang-format patch

paulwalker-arm added a comment.EditedJul 28 2022, 4:07 PM

This looks like a sensible first step to me[1]. That said, looking at the definition of getIntPtrConstant shows it has an isTarget feature so perhaps you can simplify the patch by just updating the existing getIntPtrConstant calls to set this parameter?

[1] Part of me wondered why this data is an operand at all, rather than being an SDFlag.

switch to using getIntPtrConstant isTarget=true

paulwalker-arm accepted this revision.Jul 29 2022, 8:47 AM
This revision is now accepted and ready to land.Jul 29 2022, 8:47 AM
This revision was automatically updated to reflect the committed changes.