[PATCH] D41362: [AArch64][GlobalISel] Enable GlobalISel at -O0 by default

Amara Emerson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 2 04:36:08 PST 2018

aemerson added a comment.

In https://reviews.llvm.org/D41362#965646, @Gerolf wrote:

> Hi,
> I see all issues that came up in this thread covered by the last patch. Before this can be committed I still want to check that all paths/issues are tested/covered wrt to fast-isel:
>  a) how do we guarantee that there is no fallback from GISel to FastISel (when GISel is supported)? This is probably a nit since it is obvious to everyone deeper in the implementation then I am.
>  b) for all the tests where fast-isel was added  shouldn't there equivalent tests for GISel, too? Even if the tests target fast-isel specific issues how do we make sure GISel does not expose similar/same bugs?
> Thanks
> Gerolf

Reading the code again I think we're covered for the fall back issue for the cases where `TargetOptions::EnableFastISel` is queried.

As for the tests, this change should maintain the existing test coverage of the code. You're correct in that there are cases where we're not testing the equivalent feature with global-isel, this should be follow on work after this to make GISel feature complete (rdar://35603467). I don't think adding -global-isel to the existing tests would help since we'd end up having to XFAIL them anyway. For the tests which are fast-isel specific they're mostly implementing support for something rather than bugs, and we should have tests for those as we add support over time in the GlobalISel specific tests.

Comment at: test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll:1
-; RUN: not llc -O0 -global-isel -verify-machineinstrs %s -o - 2>&1 | FileCheck %s --check-prefix=ERROR
+; RUN: not llc -O0 -global-isel -global-isel-abort=1 -verify-machineinstrs %s -o - 2>&1 | FileCheck %s --check-prefix=ERROR
 ; RUN: llc -O0 -global-isel -global-isel-abort=0 -verify-machineinstrs %s -o - 2>&1 | FileCheck %s --check-prefix=FALLBACK
Gerolf wrote:
> Nit: -global-isel is no longer needed.



More information about the llvm-commits mailing list