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

[Quentin Colombet via Phabricator via cfe-commits]

qcolombet added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:4699
+    else
+      CmdArgs.push_back("-global-isel=0");
+  }
----------------
aemerson wrote:
> 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.
> 
> 
> Yes that makes sense, although should we ignore it for ARM64 O0 since we will officially support it?

Sounds sensible. We can advertise the remark option if people want to know what is going on.


Repository:
  rC Clang

https://reviews.llvm.org/D42276