This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAGBuilder] Fix ISD::FREEZE creation for structs with fields of different types.
ClosedPublic

Authored by craig.topper on Mar 30 2020, 2:22 PM.

Details

Summary

The previous code used the type of the first field for the VT
passed to getNode for every field.

I've based the implementation here off what is done in visitSelect
as it removes the need to special case aggregates.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 30 2020, 2:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2020, 2:22 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Looks good to me, but maybe someone else who knows SelDag well should review this as well..?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
10556

Realized that the original code had a problem with a nested struct type.
It would be great if there is a test for freeze with nested struct type.

aqjune added inline comments.Mar 30 2020, 10:39 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
10556

Sorry, the original code seems to successfully deal with a nested struct type too.

craig.topper marked an inline comment as done.Mar 30 2020, 11:28 PM
craig.topper added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
10556

I think it still has a bug if the types of the other fields in the struct don't match the first type. For example this triggers an assert. (and not the one i just added..

define i64 @freeze_anonstruct() {

%y1 = freeze {i32, {i64}} undef
%v1 = extractvalue {i32, {i64}} %y1, 0
%v2 = extractvalue {i32, {i64}} %y1, 1, 0
%v3 = zext i32 %v1 to i64
%t1 = add i64 %v3, %v2
ret i64 %t1

}

Looks good to me, but maybe someone else who knows SelDag well should review this as well..?

Oh I meant just anyone other than me, including people who were already added as reviewers. :)

aqjune accepted this revision.Apr 5 2020, 5:39 PM
This revision is now accepted and ready to land.Apr 5 2020, 5:39 PM
aqjune added a comment.Apr 5 2020, 5:40 PM

Clarify that the change seems good to me

This revision was automatically updated to reflect the committed changes.