This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Manage scalarized values using VPValues.
ClosedPublic

Authored by fhahn on Nov 29 2020, 1:48 PM.

Details

Summary

This patch updates codegen to use VPValues to manage the generated
scalarized instructions.

Diff Detail

Event Timeline

fhahn created this revision.Nov 29 2020, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2020, 1:48 PM
fhahn requested review of this revision.Nov 29 2020, 1:48 PM
fhahn updated this revision to Diff 320347.Jan 31 2021, 5:32 AM

Adjust vplan printing test and split off changes to use VPTransformState in a few more places to D95757. This is required to ensure we pick up the scalarized values managed directly in VPTransformState.

a.elovikov added inline comments.Feb 1 2021, 10:35 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2697

nit: Should it be Builder.getInsertBlock() to avoid calling getParent on BasicBlock::end() iterator? It might be impossible to have the end iterator here, but the getInsertBlock version is generally safer, IMO.

2718–2719

nit: I think commenting it out would be misleading for the reader. Can we just remove it?

fhahn updated this revision to Diff 320768.Feb 2 2021, 6:09 AM

Use GetInsertBlock, remove misleading comment, thanks!

fhahn added inline comments.Feb 2 2021, 6:09 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2697

Good point, adjusted!

2718–2719

Yes, the comment should be outdated now. I removed it.

gilr added inline comments.Feb 3 2021, 1:18 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8231

Call getOperand(0)->getUnderlyingValue() directly?

8232

Can this line be brought back under the comment to minimize the diff?

8239–8248

Can the then/else logic be reversed to minimize the diff?

fhahn updated this revision to Diff 321342.Feb 4 2021, 1:12 AM

Updated code in VPPredInstPHIRecipe::execute to reduce diff as suggested. Thanks!

fhahn marked 3 inline comments as done.Feb 4 2021, 9:55 AM
gilr added inline comments.Feb 9 2021, 3:55 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8089

No need to pass as arguments now that State itself is passed.

8253

This is porting the resetVectorValue to State operations, which I think is fine by itself (I tend to agree with passing nullptr as the underlying value to VPPredInstPHI), but something else bothers me: It seems VPPredInstPHI doesn't take over the VPReplicate as the VPValue mapped to the predicated IR value during VPlan construction, which means that recipes constructed for users of predicated instructions use the ReplicateRecipe although it doesn't dominate them. We get away with this by overriding the IR's vector value, but for correct VP def-use we should have VPPredInstPHI take over as the registered VPValue in VPlan during construction. Does all that make sense?

fhahn added inline comments.Feb 9 2021, 9:53 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8253

I think the problem here is how we generate code for a scalarization region, like the one below. Uses after the region use the PHI-PREDICATED-INSTRUCTION VPValue.

But when generating code for the region itself, we execute the same blocks once for each vector element. So in the second iteration "REPLICATE ir<%div> = udiv ir<16>, ir<%counter1.0> (S->V)\l" currently needs to pack its result in the result vector from the previous iteration. Currently that is done by overwriting the state of the def "REPLICATE ir<%div> = udiv ir<16>, ir<%counter1.0> (S->V)\l" with the predicated PHI from the previous iteration, so the vector-pack logic picks it up.

It might be possible to instead change the code to just have scalar phis and do the packing as part of the phi creation. I think for transformations, the VPValue def-use chain should be correct, and it is 'just' a code-gen improvement. Do you think we need to fix this before this one goes in?

subgraph cluster_N2 {
  fontname=Courier
  label="\<xVFxUF\> pred.udiv"
  N1 [label =
    "pred.udiv.entry:\n" +
      " +
      "BRANCH-ON-MASK vp<%4>\l"\l" +
       "CondBit: vp<%4> (for.body3)\l"
  ]
  N1 -> N3 [ label="T"]
  N1 -> N4 [ label="F"]
  N3 [label =
    "pred.udiv.if:\n" +
      "REPLICATE ir<%div> = udiv ir<16>, ir<%counter1.0> (S->V)\l"
  ]
  N3 -> N4 [ label=""]
  N4 [label =
    "pred.udiv.continue:\n" +
      "PHI-PREDICATED-INSTRUCTION vp<%6> = ir<%div>\l"
  ]
}
fhahn updated this revision to Diff 323614.Feb 14 2021, 5:56 AM

Ping :)

Rebased and added a note about why we are re-setting the value for operands of VPPRedPHI. Also added a few comments to set/reset functions and assertions.

fhahn added inline comments.Feb 14 2021, 5:58 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8253

@gilr I added a note explaining why we need to also update the operand. Do you think that is sufficient to move forward with this patch?

gilr accepted this revision.Feb 15 2021, 2:43 PM
gilr added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8089

No need to pass as arguments now that State itself is passed.

Does that make sense?

8253

Yes, thanks!

8253

Uses after the region use the PHI-PREDICATED-INSTRUCTION VPValue.

Right, I missed the part where you replace the predicated instruction's VPValue with the new PredInstRecipe.

It might be possible to instead change the code to just have scalar phis and do the packing as part of the phi creation. I think for transformations, the VPValue def-use chain should be correct, and it is 'just' a code-gen improvement. Do you think we need to fix this before this one goes in?

Not sure recipes should create values in other places than their designated location. Anyway, this shouldn't block this patch.

This revision is now accepted and ready to land.Feb 15 2021, 2:43 PM
fhahn updated this revision to Diff 323903.Feb 16 2021, 1:02 AM

Thanks Gil! I removed the UF/VF arguments from widenPHIInstruction, I totally missed that I did not address this comment originally.

fhahn added inline comments.Feb 16 2021, 1:07 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8089

Does that make sense?

Yes, sorry I totally missed that I did not update this so far! Fixed now.

This revision was landed with ongoing or failed builds.Feb 16 2021, 1:08 AM
This revision was automatically updated to reflect the committed changes.