This is an archive of the discontinued LLVM Phabricator instance.

[llvm][IROutliner] Account for return void in sort comparator
ClosedPublic

Authored by DavidSpickett on Jul 21 2022, 1:38 AM.

Details

Summary

This fixes 69 llvm tests that failed when EXPENSIVE_CHECKS was enabled.
llvm/test/Transforms/IROutliner/outlining-commutative-operands-opposite-order.ll
is one example.

When we have EXPENSIVE_CHECKS, _GLIBCXX_DEBUG is defined. This means
that libstdc++ will call the compare function to check if it is
implemented correctly (that !(a < a) is true).

This happens even if there is only one item and here, we expect
to see one return void or multiple return constant integer.

Don't sort if we have 1 item, but do assert that it is the 1
ret void we expect. In the comparator, assert that neither
Value is a nullptr in case one ended up in a the list somehow.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jul 21 2022, 1:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 1:38 AM
DavidSpickett requested review of this revision.Jul 21 2022, 1:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 1:38 AM

The thing I'm not sure of here is if we should ever expect to have >1 item in the map if there is a "ret void". Or if it is one of two states:

  • one ret void
  • multiple return constant integer

I'm not familiar enough with this area to know that. If it's like that then it's probably better to say if EXPENSIVE_CHECKS defined and size is 1, skip the sort entirely.

AndrewLitteken added a comment.EditedAug 3 2022, 8:47 AM

There shouldn't be a case where there is a ret void and a ret <Constant Int> in a function. I am pretty sure that there will only more than one return value when the function has more than one exit block. In that case, there are multiple return values have an integer value. So the cases you laid out should be correct.

So it may be better to use your suggestion to only sort when there is more than one value, and add two assertions. One to check that the only value is a nullptr when the map is size 1, and then another in the sort itself that checks that neither the LHS or the RHS is a nullptr.

DavidSpickett planned changes to this revision.Aug 3 2022, 8:51 AM

Thanks for explaining, I'll update this shortly.

Implement suggested changes.

DavidSpickett edited the summary of this revision. (Show Details)Aug 4 2022, 2:40 AM
AndrewLitteken accepted this revision.Aug 4 2022, 8:37 AM

LGTM with small nit.

llvm/lib/Transforms/IPO/IROutliner.cpp
175

small nit, you could add an early return here, and then remove the else statement.

This revision is now accepted and ready to land.Aug 4 2022, 8:37 AM

Use early return.

DavidSpickett marked an inline comment as done.Aug 5 2022, 2:35 AM
This revision was landed with ongoing or failed builds.Aug 5 2022, 2:36 AM
This revision was automatically updated to reflect the committed changes.