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

[Amara Emerson via Phabricator via llvm-commits]

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.
Ack.


Repository:
  rL LLVM

https://reviews.llvm.org/D41362