This is an archive of the discontinued LLVM Phabricator instance.

[LLVM] Merge -preserve-ll-uselistorder/preserve-bc-uselistorder into -preserve-uselistorder
Needs RevisionPublic

Authored by aeubanks on Nov 28 2022, 11:44 AM.

Details

Reviewers
dexonsmith
Summary

It seems unnecessary to have separate flags for assembly vs bitcode.

Diff Detail

Event Timeline

aeubanks created this revision.Nov 28 2022, 11:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 11:44 AM
aeubanks requested review of this revision.Nov 28 2022, 11:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 11:44 AM
dexonsmith requested changes to this revision.Nov 28 2022, 1:44 PM

Separate flags allows vendors (and the default!) to have use-list order on-by-default in bitcode and off-by-default in textual IR.

IMO, the best path to simplification here:

  1. Make it illegal or impossible to observe the use-lists of ConstantData (most importantly, ConstantInt and ConstantFloat). E.g., change replaceAllUsesWith() to crash for those types and require people to check first.
  2. Stop serializing the use-lists of ConstantData (since we know there are no observers). This removes 99% of (a) the space overhead in bitcode and (b) the noise in textual assembly, since we're no longer tracking the use-list of i1 0, i1 1, i8 0, etc.
  3. Consider merging the two options and flipping them both on by default since the noise in the textual IR is no longer terrible?
  4. Consider removing -preserve-bc-use-list-order and just turn it on all the time since it no longer has major overhead?
  5. Consider removing -preserve-ll-use-list-order and just turn it on all the time since it no longer adds undue noise to the IR?

Note that walking the uses of i1 0 (or similar) is dangerous since it's operating on all functions/modules/etc. in the LLVMContext. Any code that's doing this should probably be rewritten for other soundness reasons.

I used to have a series of patches to the above effect; there were only a few more test failures to fix before I could land it. Unfortunately I dropped the work since I got busy with something else. Here were some of the fixes I landed:

  • r263853 / c3fa1eded258aa38a30911af6602892846efd6d1 (this one was pretty bad)
  • r282320 / c82c11428e0ab2ff58a342ca3a615315929a2633
  • r282333 / b4798739127cd7748177ac506e6d45cc8cf5e4f8
  • r282334 / 4fd9b7e16f98173d730ac1a347f7eae96a9fd2db
  • r282337 / b1b208a1f514ccb90b5afba0af49d6aa39dee21b
  • r282338 / 11c06ea55a060d5460f716b00ca593e5dd829b17
llvm/tools/opt/opt.cpp
217

Notice that bitcode defaults to on...

222

... but textual assembly defaults to off.

This revision now requires changes to proceed.Nov 28 2022, 1:44 PM

Here's a link to a related RFC:
https://discourse.llvm.org/t/rfc-constantdata-should-not-have-use-lists/42606

Patch 0003 removed use-lists from ConstantData; likely no longer applies, but possibly not too hard to rebase.

At the time, @mehdi_amini and @chandlerc both favoured a subsequent follow up to change the type hierarchy. I agree! But it's not necessary for the initial step.