This is an archive of the discontinued LLVM Phabricator instance.

[NFC][DebugInfo][RemoveDIs] Prefer insert-with-iterator over instruction pointers when inserting into blocks
ClosedPublic

Authored by jmorse on Jun 9 2023, 7:32 AM.

Details

Summary

Continuing the patches from my proposal for how to get rid of debug intrinsics [0], we would also need to have most instruction insertion routines take an iterator instead of just an instruction pointer -- so that in the future we can communicate more information about debug-info in the iterator class. This patch adds an iterator-taking insertBefore method, and converts a raft of call-sites to using it instead of inserting through instruction-constructors or similar. Again, these are all the call-sites that had to be instrumented to make a stage2clang build identically, plus a couple of other large C++ code bases I have to hand.

I suspect the ultimate outcome of this would be making all the inserter functions take iterators.

Patch is technically NFC and I'm not adding reviewers; this is so that people can take a look at what changes I'm proposing, and have a think about how we can go about testing them.

[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939

Diff Detail

Event Timeline

jmorse created this revision.Jun 9 2023, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2023, 7:32 AM
jmorse requested review of this revision.Jun 9 2023, 7:32 AM
Orlando accepted this revision.Sep 8 2023, 1:40 AM
Orlando added a subscriber: Orlando.

There are a load of getFirstNonPHI() -> getFirstInsertionPt() that I think should be getFirstNonPHI() -> getFirstNonPHIIt() (marked inlined) based on the discussion in D152468, otherwise LGTM.

llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp
711
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
2202
llvm/lib/Transforms/IPO/PartialInlining.cpp
1056
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
2142–2143

nit: Looks like this bit has just been reformatted? Possibly should be left unchanged, but not a strong opinion.

llvm/lib/Transforms/Utils/CodeExtractor.cpp
781
llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
109
273
368
917
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7916
This revision is now accepted and ready to land.Sep 8 2023, 1:40 AM
This revision was landed with ongoing or failed builds.Sep 11 2023, 3:49 AM
This revision was automatically updated to reflect the committed changes.

Cheers; corrections applied when landing. I've merged in getFirstNonPHIIt in this patch from an earlier approved one, as the earlier one has extra dependencies.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
2142–2143

To confirm, yup that's clang-format doing stuff, I'll undo it to avoid future archaeologists being confused