Page MenuHomePhabricator

[GlobalISel] Fix choice of instruction selector for AArch64 at -O0 with -global-isel=0

Authored by petpav01 on Jan 3 2019, 5:12 AM.



Commit rL347861 introduced an unintentional change in the behaviour when compiling for AArch64 at -O0 with -global-isel=0. Previously, explicitly disabling GlobalISel resulted in using FastISel but an updated condition in the commit changed it to using SelectionDAG. The patch fixes this condition and slightly better organizes the code that chooses the instruction selector.

Fixes PR40131.

Note that a fix without any cleanup would look as follows:

diff --git a/lib/CodeGen/TargetPassConfig.cpp b/lib/CodeGen/TargetPassConfig.cpp
index defb165fe0f..7a1747a6ad8 100644
--- a/lib/CodeGen/TargetPassConfig.cpp
+++ b/lib/CodeGen/TargetPassConfig.cpp
@@ -757,7 +757,8 @@ bool TargetPassConfig::addCoreISelPasses() {
   TM->setO0WantsFastISel(EnableFastISelOption != cl::BOU_FALSE);
   if (EnableFastISelOption == cl::BOU_TRUE ||
       (TM->getOptLevel() == CodeGenOpt::None && TM->getO0WantsFastISel() &&
-       !TM->Options.EnableGlobalISel)) {
+       (EnableGlobalISelOption == cl::BOU_FALSE ||
+        !TM->Options.EnableGlobalISel))) {

Diff Detail


Event Timeline

petpav01 created this revision.Jan 3 2019, 5:12 AM
Hahnfeld added a subscriber: Hahnfeld.

I don't know anything about GlobalISel implementation. The patch seems to do the right thing, but I'll defer to somebody who knows the code.

This also looks fine to me but I'm in the same boat as Jonas when it comes to (non-)knowledge of GlobalISel.

Thanks for the cleanup. LGTM with minor naming change.

760 ↗(On Diff #180039)

Minor nit: can we maybe rename Selector->SelectorType and then properly capitalize the 'selector' variable.

aemerson accepted this revision.Jan 7 2019, 1:24 PM
This revision is now accepted and ready to land.Jan 7 2019, 1:24 PM

Thank you for the review. I will commit the patch with the suggested change shortly.

This revision was automatically updated to reflect the committed changes.