This is an archive of the discontinued LLVM Phabricator instance.

[ScheduleDAGInstrs] fix behavior of getUnderlyingObjectsForCodeGen when no identifiable object found
ClosedPublic

Authored by inouehrs on Oct 10 2017, 8:05 AM.

Details

Summary

This patch fixes the bug introduced in my previous patch https://reviews.llvm.org/D35907 reported by http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171002/491452.html .

Before D35907, when GetUnderlyingObjects fails to find an identifiable object, allMMOsOkay lambda in getUnderlyingObjectsForInstr returns false and Objects vector is cleared. This behavior is unintentionally changed by D35907.

This patch makes the behavior for such case same as the previous behavior.
Since D35907 introduced a wrapper function getUnderlyingObjectsForCodeGen around GetUnderlyingObjects, getUnderlyingObjectsForCodeGen is modified to return a boolean value to ask the caller to clear the Objects vector.

Diff Detail

Event Timeline

inouehrs created this revision.Oct 10 2017, 8:05 AM
MatzeB edited edge metadata.Oct 10 2017, 10:14 AM

This looks good, but makes me wonder if we shouldn't do the same change (returning a bool instead of looking at the vector containing 0 or more elements) to getUnderlyingObjectsForInstr() for consistency. Could you try doing that?

[[ https://github.com/llvm-mirror/llvm/blob/c19eec32627989457c78dbfd91f79c3be1b99422/lib/CodeGen/ScheduleDAGInstrs.cpp#L187-L223 | In the old implementation of getUnderlyingObjectsForInstr ]], Objects vector is cleared when getUnderlyingObjects (and hence GetUnderlyingObjects) returns an unidentified object, but not cleared when getUnderlyingObjects returns no object.
I added boolean return value in getUnderlyingObjectsForCodeGen to distinguish these two cases, since it returns empty vector for both cases.

Do you mean using boolean return value also in getUnderlyingObjectsForInstr for consistency?

Do you mean using boolean return value also in getUnderlyingObjectsForInstr for consistency?

Yes that's what I meant.

inouehrs updated this revision to Diff 118737.Oct 11 2017, 10:00 PM

@MatzeB Thank you for the clarification. How about this?

MatzeB accepted this revision.Oct 11 2017, 10:07 PM

Thanks, LGTM

This revision is now accepted and ready to land.Oct 11 2017, 10:07 PM
This revision was automatically updated to reflect the committed changes.