Page MenuHomePhabricator

[VPlan] Add Debugloc to VPInstruction.
ClosedPublic

Authored by fhahn on Dec 5 2021, 12:30 PM.

Details

Summary

Upcoming changes require attaching debug locations to VPInstructions,
e.g. adding induction increment recipes in D113223.

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

fhahn created this revision.Dec 5 2021, 12:30 PM
fhahn requested review of this revision.Dec 5 2021, 12:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2021, 12:30 PM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added a comment.Dec 13 2021, 1:21 AM

Could actual DLs be conveyed, to exercise it, e.g., by associating the DL of ICmpULE with the comparison controlling the original loop?
Finding a DL for FirstOrderRecurrenceSplice seems hard(er), perhaps the instruction feeding the next iteration(?).
(Avoid defaulting DL to none in the spirit of MLIR?)

fhahn updated this revision to Diff 393827.Dec 13 2021, 3:13 AM

Could actual DLs be conveyed, to exercise it, e.g., by associating the DL of ICmpULE with the comparison controlling the original loop?
Finding a DL for FirstOrderRecurrenceSplice seems hard(er), perhaps the instruction feeding the next iteration(?).
(Avoid defaulting DL to none in the spirit of MLIR?)

I think for the uses in D113223 there already is a debug location in all cases. The reason for making it optional were other places where VPInstructions are created. But I updated the patch to make it non-optional and push the responsiblity to the places where instructions are created. I think in most cases we potentially have a location available and I tried to use it.

Ayal added a comment.Dec 13 2021, 3:33 AM

Could actual DLs be conveyed, to exercise it, e.g., by associating the DL of ICmpULE with the comparison controlling the original loop?
Finding a DL for FirstOrderRecurrenceSplice seems hard(er), perhaps the instruction feeding the next iteration(?).
(Avoid defaulting DL to none in the spirit of MLIR?)

I think for the uses in D113223 there already is a debug location in all cases. The reason for making it optional were other places where VPInstructions are created.

Yes, this motivation of D113223 is clear from the summary.

But I updated the patch to make it non-optional and push the responsiblity to the places where instructions are created. I think in most cases we potentially have a location available and I tried to use it.

Very good! Is it possible to test?

fhahn updated this revision to Diff 393859.Dec 13 2021, 5:11 AM

But I updated the patch to make it non-optional and push the responsiblity to the places where instructions are created. I think in most cases we potentially have a location available and I tried to use it.

Very good! Is it possible to test?

Yes! I precommitted both a codegen test and a VPlan printing test. The changes are now included. The patch is not NCF any longer, I'll update the description accordingly.

fhahn retitled this revision from [VPlan] Add Debugloc to VPInstruction (NFC). to [VPlan] Add Debugloc to VPInstruction..Dec 13 2021, 5:11 AM
Ayal accepted this revision.Dec 13 2021, 12:26 PM

Thanks! Looks good to me.

This revision is now accepted and ready to land.Dec 13 2021, 12:26 PM
fhahn updated this revision to Diff 394278.Dec 14 2021, 8:45 AM

rebase before committing

This revision was landed with ongoing or failed builds.Dec 20 2021, 7:11 AM
This revision was automatically updated to reflect the committed changes.