This is an archive of the discontinued LLVM Phabricator instance.

[ConstraintElimination] Change debug output to display variable names.
ClosedPublic

Authored by zjaffal on Jan 26 2023, 5:34 AM.

Details

Summary

Previously when constraint system outputs the rows in the system the variables used are x1,2...n making it hard to infer which ones they relate to in the IR

Diff Detail

Event Timeline

zjaffal created this revision.Jan 26 2023, 5:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 5:34 AM
zjaffal requested review of this revision.Jan 26 2023, 5:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 5:34 AM

Thanks for looking into this! Would it make sense to change ConstraintSystem to hold a pointer/reference to the corresponding variable map and then isolate the name generation logic and dumping to ConstraintSystem.cpp only?

nikic added a comment.Jan 26 2023, 5:43 AM

We should make sure that no name map is generated when not debugging.

zjaffal updated this revision to Diff 492488.Jan 26 2023, 9:29 AM

Refactored the printing logic to be done in ConstraintSystem

fhahn added a comment.Feb 6 2023, 2:18 PM

Would it be possible to add a test to show the additional dbg output, e.g. llvm/test/Transforms/ConstraintElimination/debug.ll?

llvm/include/llvm/Analysis/ConstraintSystem.h
38

capitalize first letter of sentence and period at end of sentence. Newline before comment

39

Please run clang-format and remove the needed ;

40

Pass argument as const reference?

llvm/lib/Analysis/ConstraintSystem.cpp
111

Can use (auto &[V, Index] : Value2Index).

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
660

This function isn't needed any longer.

663

Change to const reference and adjust name?

zjaffal updated this revision to Diff 495845.Feb 8 2023, 7:27 AM
zjaffal marked 6 inline comments as done.
  1. Modify debug.ll test to display variable names.
  2. Delete unused functions and rename dumpdumpWithNames to dumpConstraint
  3. Address style comments
fhahn added inline comments.Feb 8 2023, 7:53 AM
llvm/include/llvm/Analysis/ConstraintSystem.h
38

still needs a newline before the comment.

40

not just const, const reference, i.e. const DenseMap<Value *, unsigned> &Value2Index

41

newline before the next function.

41

; here also needs removed and reformatting. Please run clang-format.

95

The bit about using Names needs to be removed.

zjaffal marked 5 inline comments as done.Feb 9 2023, 3:56 AM
zjaffal updated this revision to Diff 496079.Feb 9 2023, 3:57 AM

fix formatting and update dump() function comment

fhahn accepted this revision.Feb 10 2023, 7:44 AM

LGTM, with a few small nits addressed.

llvm/include/llvm/Analysis/ConstraintSystem.h
38

nit: from *the* Value2Index map.

llvm/lib/Analysis/ConstraintSystem.cpp
112

nit: the coding style recommends using auto sparingly and spell out full types unless they are too big. So here it would probably be better to use SmallVector<std::string>

121

nit: add newline here.

This revision is now accepted and ready to land.Feb 10 2023, 7:44 AM
zjaffal updated this revision to Diff 497666.Feb 15 2023, 7:05 AM
zjaffal marked an inline comment as done.

Address nit comments

zjaffal marked 2 inline comments as done.Feb 15 2023, 7:06 AM
This revision was landed with ongoing or failed builds.Feb 15 2023, 7:08 AM
This revision was automatically updated to reflect the committed changes.