Page MenuHomePhabricator

[wasm] Fix FastISel generating NoReg

Authored by ddcc on Aug 2 2016, 7:35 PM.



Previously, FastISel for WebAssembly wasn't checking the return value of getRegForValue in certain cases, which would generate instructions referencing NoReg. This patch fixes this behavior.

Diff Detail


Event Timeline

ddcc updated this revision to Diff 66609.Aug 2 2016, 7:35 PM
ddcc retitled this revision from to [wasm] Fix FastISel generating NoReg.
ddcc updated this object.
ddcc added a subscriber: llvm-commits.
dschuff added inline comments.Aug 3 2016, 11:14 AM
1 ↗(On Diff #66609)

This command line shouldn't result in fast-isel even running, should it?

11 ↗(On Diff #66609)

are these constexprs (and inttoptrs/casts) necessary to reproduce?
Is the underlying problem is that the getRegForValue calls for the icmp are bailing, or something like that?

ddcc added inline comments.Aug 3 2016, 2:30 PM
1 ↗(On Diff #66609)

Portions of this pass are getting called through e.g. the generic FastISel.cpp:1417 and SelectionDAGISel.cpp:1369 for selecting return instructions.

11 ↗(On Diff #66609)

Yes, I believe they are necessary. The issue is that when optimization has been turned off in the front-end, instruction can have sufficiently complex operands that fail inside getRegForValue during FastISel, which will return zero. But previously we weren't checking these return values, putting NoReg into the generated instructions and causing problems later.

dschuff added inline comments.Aug 3 2016, 3:43 PM
1 ↗(On Diff #66609)

Hm, I didn't realize that any of this ran when not using fast-isel. In any case we should also add a second RUN line that includes -fast-isel to explicitly get as much of fast-isel as possible.

3 ↗(On Diff #66609)

This description doesn't really make sense; the fact that NoReg operands are generated is the bug. Really what is happening is that there are parts that cannot be selected by fast-isel but it doesn't properly bail out.

dschuff added inline comments.Aug 3 2016, 3:52 PM
3 ↗(On Diff #66609)

Oh, probably the corollary to "fix the comment to say what the tested condition and expected result is" would also be to change the name of the test file to something that reflects the tested condition.

ddcc updated this revision to Diff 66819.Aug 4 2016, 9:27 AM

Rename test, update comment, add -fast-isel

dschuff accepted this revision.Aug 4 2016, 10:28 AM
dschuff edited edge metadata.

OK, looks good. one more request, please expand the commit description a little bit.

This revision is now accepted and ready to land.Aug 4 2016, 10:28 AM
ddcc updated this object.Aug 4 2016, 10:58 AM
ddcc edited edge metadata.
This revision was automatically updated to reflect the committed changes.