[PATCH] D42276: [Driver] Add an -fexperimental-isel driver option to enable/disable GlobalISel

[Amara Emerson via Phabricator via cfe-commits]

aemerson added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:4699
+    else
+      CmdArgs.push_back("-global-isel=0");
+  }
----------------
qcolombet wrote:
> qcolombet wrote:
> > I think that it would be useful to also set -global-isel-abort=2, so that users can report problem early.
> > 
> > What do you think?
> > 
> > 
> Should we have some kind of "target validation"?
> What I'd like to avoid is people trying the new allocator on unsupported target (as in the GISel base classes are not present) that would just crash the compiler.
> 
> Alternatively, we could fix the backend to fallback gracefully/abort properly in those situation.
> Right now I believe we would get a segfault on RegisterBankInfo or something along those lines.
> I think that it would be useful to also set -global-isel-abort=2, so that users can report problem early.
> 
> What do you think?

Yes that makes sense, although should we ignore it for ARM64 O0 since we will officially support it?

> 
> Should we have some kind of "target validation"?
> What I'd like to avoid is people trying the new allocator on unsupported target (as in the GISel base classes are not present) that would just crash the compiler.

Perhaps a warning like "GlobalISel support is incomplete for [target]"? I don't know the GISel status for other targets.




Repository:
  rC Clang

https://reviews.llvm.org/D42276