Page MenuHomePhabricator

[ARM] Remove exit-on-error flag from test (PR27765)
ClosedPublic

Authored by rovka on Jun 7 2016, 2:14 AM.

Details

Summary

The exit-on-error flag is necessary in order to avoid an unreachable in the DAGTypeLegalizer, when trying to expand a physical register. We can also avoid this situation by introducing a bitcast early on, where the invalid scalar-to-vector conversion is detected.

Fixes PR27765.

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 59846.Jun 7 2016, 2:14 AM
rovka retitled this revision from to [ARM] Remove exit-on-error flag from test (PR27765).
rovka updated this object.
rovka added reviewers: t.p.northover, bogner, echristo.
rovka added subscribers: llvm-commits, rengolin.
rengolin added inline comments.Jun 8 2016, 4:47 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
452 ↗(On Diff #59846)

I take it that this transformation is harmless, since the compilation is going to fail anyway.

But I'm worried that this could trigger unrelated errors down the pipe until the last error is reported and the program exits, and confuse the user.

rovka added inline comments.Jun 8 2016, 5:15 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
452 ↗(On Diff #59846)

Not taking this lightly, but we're not seeing any other errors now, and I'm not sure how you could recover in a way that would guarantee you won't see any bogus errors ever (while still reporting all the useful ones). I think it should be pretty safe to have a bitcast between two values that have the same number of bits (assertion just above, at line 445).

I'm open to other suggestions though :)

rengolin added inline comments.Jun 8 2016, 5:22 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
452 ↗(On Diff #59846)

Right... maybe @bogner or @echristo can shed some light?

rovka updated this revision to Diff 60046.Jun 8 2016, 8:29 AM

Updated the test to use something other than undef as input too. This way we can test 2 different code paths (the one for undef is a bit special because it really doesn't create a bitcast node, it just creates an undef with the type we're bitcasting to). With this change, we're testing something a bit more realistic, and the fix still seems to work.

I can't find any tests for this error on other targets. Should I try to add one at least for X86?

Updated the test to use something other than undef as input too. This way we can test 2 different code paths (the one for undef is a bit special because it really doesn't create a bitcast node, it just creates an undef with the type we're bitcasting to). With this change, we're testing something a bit more realistic, and the fix still seems to work.

Right, good idea. That gives me more confidence on the patch. :)

I can't find any tests for this error on other targets. Should I try to add one at least for X86?

If you can, that'd be great. It would ease my worries about other arches.

cheers,
--renato

rovka added a comment.Jun 10 2016, 1:58 AM

I can't find any tests for this error on other targets. Should I try to add one at least for X86?

If you can, that'd be great. It would ease my worries about other arches.

Ok, working on it.

However, I think your concern is more general - on any test where we check for one error, we might want to make sure we don't trigger other bogus errors down the line. Should all our tests that check for one error also make sure there aren't any unexpected errors issued? I can put that in my backlog if it sounds like something we want.

However, I think your concern is more general - on any test where we check for one error, we might want to make sure we don't trigger other bogus errors down the line. Should all our tests that check for one error also make sure there aren't any unexpected errors issued? I can put that in my backlog if it sounds like something we want.

I think it's impossible to predict everything. I'm happy with some other arch's test for now.

rovka updated this revision to Diff 60352.Jun 10 2016, 7:47 AM

Added a test for PowerPC.

X86 proved a bit tricky, because there is some special code in X86TargetLowering::getRegForInlineAsmConstraint in case the generic getRegForInlineAsmConstraint didn't return what X86 wanted. It even has this comment:
Handle references to XMM physical registers that got mapped into the
wrong class. This can happen with constraints like {xmm0} where the
target independent register mapper will just pick the first match it can
find, ignoring the required type.

So I'm guessing X86 is trying pretty hard to avoid this code path, so even if I managed to make it go through there it would probably be a bug.

PowerPC on the other hand seems to be handling things more similarly to ARM, and it can also cope with the bitcast that this patch is adding.

rengolin accepted this revision.Jun 10 2016, 7:55 AM
rengolin added a reviewer: rengolin.

LGTM, thanks!

This revision is now accepted and ready to land.Jun 10 2016, 7:55 AM
rovka added a comment.Jun 10 2016, 7:57 AM

Ok, thanks, I'll leave this open for a few more days in case other people want to chime in (especially from other targets).

This revision was automatically updated to reflect the committed changes.