Page MenuHomePhabricator

[SelectionDAG] reset NewNodesMustHaveLegalTypes flag between basic blocks
ClosedPublic

Authored by guyblank on May 23 2017, 3:37 AM.

Details

Summary

The NewNodesMustHaveLegalTypes flag is set to false at the beginning of CodeGenAndEmitDAG, and set to true after legalizing types.
But before calling CodeGenAndEmitDAG we build the DAG for the basic block. So for the first basic block NewNodesMustHaveLegalTypes would be 'false' during the SDAG building, and for all other basic blocks it would be 'true'.

This patch sets the flag to false before SDAG building each basic block.

While the patch affects an AArch64 test, the test would produce the same result in trunk if moved to be the first function in the file. So someone familiar with the AArch64 codegen might want to take a look at the test regardless.

Diff Detail

Repository
rL LLVM

Event Timeline

guyblank created this revision.May 23 2017, 3:37 AM
RKSimon added inline comments.May 23 2017, 7:06 AM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
639 ↗(On Diff #99860)

Move to the top and comment this properly.

guyblank updated this revision to Diff 100044.May 24 2017, 12:10 AM
guyblank marked an inline comment as done.
RKSimon edited edge metadata.May 24 2017, 8:12 AM

Any AARCH64 people want to check the test?

I'm happy with this change but an AARCH64 specialist should review the test's new codegen

@ahatanak
seeing that you're doing AArch64 codegen commits, can you please take a look at this?

ahatanak edited edge metadata.Jun 1 2017, 10:43 PM

I think what this patch is doing is correct, but it seems to me that the generated code got worse.

It looks like AArch64TargetLowering::LowerBUILD_VECTOR turns vector (0,0,-1,0,0,0,0,0) into a constant pool load instead of using immediate moves because the vector type is v8i16, not v8i8. This probably didn't happen previously because NewNodesMustHaveLegalTypes was true during legalization of the second function.

I don't know the right way to fix it (maybe you can ask @t.p.northover?).

@t.p.northover
does committing this patch and opening a PR for the AArch64 Isel issue sound reasonable?

@t.p.northover Are you happy for the AArch64 codegen regression to be raised as a bug?

zvi edited edge metadata.Jul 4 2017, 10:32 AM

@t.p.northover, being listed as AArch64's code owner, can you please assign a person to review this patch? Thanks.

RKSimon added a subscriber: fhahn.

@t.p.northover @fhahn Any comments?

t.p.northover accepted this revision.Jul 31 2017, 10:33 AM

I'm really sorry this took so long. This patch is obviously an improvement. A PR against AArch64 would be sort of nice, but I don't think any more is needed because the actual chances of it affecting real codegen seems pretty small (significant vector code in the entry block?).

This revision is now accepted and ready to land.Jul 31 2017, 10:33 AM
This revision was automatically updated to reflect the committed changes.