This is an archive of the discontinued LLVM Phabricator instance.

Change the fast-isel-abort option from bool to int to enable "levels"
ClosedPublic

Authored by mehdi_amini on Feb 27 2015, 8:38 AM.

Details

Summary

Currently fast-isel-abort will only abort for regular instructions,
and just warn for function calls, terminators, function arguments.
There is already fast-isel-abort-args but nothing for calls and
terminators.

This change turns the fast-isel-abort options into an integer option,
so that multiple levels of strictness can be defined.
This will help no being surprised when the "abort" option indeed does
not abort, and enables the possibility to write test that verifies
that no intrinsics are forgotten by fast-isel.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Change the fast-isel-abort option from bool to int to enable "levels".
mehdi_amini updated this object.
mehdi_amini edited the test plan for this revision. (Show Details)
mehdi_amini added a reviewer: resistor.
mehdi_amini added a subscriber: Unknown Object (MLST).
resistor added inline comments.Feb 27 2015, 9:48 AM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
175

This usage string looks wrong. The effect of 2 is never defined.

echristo edited edge metadata.Feb 27 2015, 9:50 AM

Some inline comments.

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
171

Can we make this default to the current default behavior? It'll mean that you don't need to update testcases too :)

175

Did you mean 2 on this line? :)

418

> 0?

1255

Don't need quotes here. (Though should probably fix from where you copied it as well :)

mehdi_amini added inline comments.Feb 27 2015, 9:59 AM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
171

The existing default value is "off". I believe it is gonna be the same here.
I had to update the tests because previously you would be able to just add -fast-isel-abort while now you need -fast-isel-abort=1
I don't know if there is a way to say that -fast-isel-abort with no argument will be like =1.
Even if it is possible, I'm not even sure it is desirable as reading a command line with -fast-isel-abort alone makes me expect to always abort.

175

Nicely spotted, it is indeed 2 on the second line.

mehdi_amini edited edge metadata.

Update thanks to Eric Christopher's review.

One reply inline and one small cosmetic comment. Otherwise LGTM.

-eric

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
171

Right. Was just trying to keep the command line from changing, but you've convinced me. :)

418

Probably want some nice whitespace, but that's cosmetic.

mehdi_amini accepted this revision.Feb 27 2015, 10:34 AM
mehdi_amini added a reviewer: mehdi_amini.
This revision is now accepted and ready to land.Feb 27 2015, 10:34 AM
mehdi_amini closed this revision.Feb 27 2015, 10:35 AM

r230775 (last cosmetic comment included)