This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Replace FMF in VPInstruction with VPRecipeWithIRFlags (NFC).
ClosedPublic

Authored by fhahn on Aug 4 2023, 2:48 PM.

Details

Summary

Update VPInstruction to use VPRecipeWithIRFlags to manage FMFs for
VPInstruction.

A follow-up patch will also use VPRecipeWithIRFlags to remove the
CanonicalIVIncrementForPartNUW and CanonicalIVIncrementNUW opcodes.

Diff Detail

Event Timeline

fhahn created this revision.Aug 4 2023, 2:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 2:48 PM
fhahn requested review of this revision.Aug 4 2023, 2:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 2:48 PM
Herald added subscribers: wangpc, vkmr. · View Herald Transcript

Thanks!. Have you ever considered to add something like getCalledFunction() when Opode == Instruction::Call to identify vscale?

fhahn added a comment.Aug 5 2023, 8:50 AM

Thanks!. Have you ever considered to add something like getCalledFunction() when Opode == Instruction::Call to identify vscale?

AFAIK vscale isn't currently modeled as VPInstruction yet, but once it does that certainly makes sense, thanks!

Thanks!. Have you ever considered to add something like getCalledFunction() when Opode == Instruction::Call to identify vscale?

AFAIK vscale isn't currently modeled as VPInstruction yet, but once it does that certainly makes sense, thanks!

Given its importance maybe model it as a dedicated derived class of VPInstruction, e.g., VPVscaleInstruction.

Ayal added inline comments.Aug 6 2023, 5:03 AM
llvm/lib/Transforms/Vectorize/VPlan.h
833

Add a constructor and/or operator= from llvm::FastMathFlags?

857

Is this change independent of this patch?

857

Could FMFs be set at construction time, only, similar to other flags?

864

Can this also be simplified into IterT alone?

879
OpType = OperationType::FPMathOp;
FMFs = Op->getFastMathFlags();

?

951–952

Worth documenting. Can be called if OpType is other than FPMathOp, but then returns arbitrary results?

Possibly worth taking out of line to VPlan.cpp

1024

Can this be another constructor rather than static member, one that takes an FMF as another parameter?

1025

Should clone also copy the flags? (Independent of this patch)

fhahn updated this revision to Diff 547719.Aug 7 2023, 4:43 AM
fhahn marked 8 inline comments as done.

Address latest comments, thanks!

llvm/lib/Transforms/Vectorize/VPlan.h
833

Added, thanks!

857

Yes updated the patch! This requires adding new constructors for both VPRecipeWithIRFlags and VPInstruciton (and the same for each variant (i.e. wrap flags and others), but should be more in line with the existing mechanisms.

857

Split off, will submit separately, thanks!

864

Yes, Split off, will submit separately, thanks!

879

Simplified, thanks!

951–952

Done in 0b17e9d2859a with an assertion added.

1024

Updated to use separate constructor, thanks!

1025

Yes this needs updating, will do separately.

Ayal added inline comments.Aug 7 2023, 3:11 PM
llvm/lib/Transforms/Vectorize/VPlan.h
865

(Nice to see I is not set/needed as underlying?)

892

Add VPInstruction?

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
387

Suffice to assert that Opcode is one having FMF, only when the Opcode and FMF are provided to VPInstruction's constructor, and its OpType is set?

Check inspired by / should match / worth comment FPMathOperator:: classof()?

397

Better have isFPMathOp() check if OpType == OperationType::FPMathOp, as done in getFastMathFlags()'s assert (which it tries to guard)?

(If so) Better have isFPMathOp() in VPRecipeWithIRFlags rather than VPInstruction, to assist potential future callers of VPRecipeWithIRFlags::getFastMathFlags()?

fhahn marked 4 inline comments as done.Aug 8 2023, 5:23 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
865

Yes, at the moment VPInstructions are completely independent of any underlying IR.

892

Added in af635a5547ec.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
387

Added a note about the relation to FPMathOperator::classof() and the current differences. I've kept the function for now, as per my response to the comment below.

397

At the moment, the subclasses of VPRecipeWithIRFlags don't have direct access to its internal fields, including OpTypes and it's the subclasses responsibility to make sure they don't access flags that are not available for the recipe.

I think it would probably be worth keeping things locked down that way for now and check directly in VPInstruction if it is an op that has fast-math flags (we do something similar for the CanonicalIVIncrement. WDYT?

fhahn updated this revision to Diff 548165.Aug 8 2023, 5:23 AM
fhahn marked 4 inline comments as done.

Address comments, thanks!

Ayal added inline comments.Aug 8 2023, 8:30 AM
llvm/lib/Transforms/Vectorize/VPlan.h
865

(Then perhaps it's setUnderlyingInstr() below can be dropped if unused.)

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
397

Keeping OpTypes and fields private and locked down inside VPRecipeWithIRFlags is fine, suggest to provide a public VPRecipeWithIRFlags::isFPMathOp() { return OpType == OperationType::FPMathOp; } in order to support callers of public VPRecipeWithIRFlags::getFastMathFlags(), efficiently.

Can have a private VPInstruction::isFPMathOp() (or isFPMathOpcode()? Under #ifndef NDEBUG?) to support the assert upon construction only, less efficiently.

Sounds right?

fhahn updated this revision to Diff 548246.Aug 8 2023, 8:53 AM
fhahn marked an inline comment as done.

Add hasFastMathFlags helper

fhahn marked an inline comment as done.Aug 8 2023, 8:55 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
865

Ah right sorry it is needed for some recipes that use this constructor, just not for VPInstructions which use the different one.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
397

Added VPRecipeWithIRFlags::hasFastMathFlags() and use it during execute. Also added an assert using isFPMathOp to make sure they don't diverge (the original reason for using isFPMathOp).

fhahn updated this revision to Diff 548249.Aug 8 2023, 8:57 AM
fhahn marked an inline comment as done.

Fix build error, guard isFPMathOp with ifdef as it's only used in assertions.

Ayal accepted this revision.Aug 8 2023, 11:06 AM

Looks good to me! Adding couple of final minor nits.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
235

nit: redundant empty line

396

nit: this is if and only if, can assert(hasFastMathFlags() == isFPMathOp() && "...").

This revision is now accepted and ready to land.Aug 8 2023, 11:06 AM
This revision was landed with ongoing or failed builds.Aug 8 2023, 12:13 PM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.