Codegen pass configs dependent on function attributes?

Hi all.

I’m trying to get GlobalISel to work better with LTO. At the moment if you enable it via -fglobal-isel, it only adds the -mllvm -global-isel and related options to the cc1 invocation. With LTO, that doesn’t work as we need to encode codegen options into the bitcode, usually via function attributes.

Does anyone have any ideas on how to achieve this? The only way I can see it working is if we have a unified codegen pipeline for both GISel and SelectionDAG. Then use a function attribute to tell the GISel passes to skip and leave it to FastISel/SelectionDAG which runs afterwards. Likewise, FastISel/SelectionDAG would need to skip the function if it was marked for GISel compilation and we didn’t trigger a fallback.

Amara

Hi Amara,

I’ve done that internally for testing purposes and basically the existing GISel pipeline with fallbacks just did what I wanted.
In other words, I always used the GISel pipeline but I added an attribute that triggered a fallback to SDISel for the functions I wanted to compile with SDISel. I had to disable the global-isel-abort thing though. That said, that should be easily fixable.

For the record, I was doing the attribute check in lowerFormalArgument IIRC. I.e., something that is called for every single function at the beginning of the GISel pipeline.

Cheers,
-Quentin

I’ve put up a patch here: https://reviews.llvm.org/D79769 that adds a unified pipeline that targets can opt-into. It has some similarities with forcing fallbacks, but uses a different mechanism to do so to preserve the abort behavior. It therefore requires that every GISel pass needs to explicitly check whether the GISel selector is being requested rather than just using the FailedISel property.

Cheers,
Amara

If you don’t care about being able to select GlobalISel vs SDag separately per object file, it’d be a lot simpler to just also pass a flag to the linker, right?

If you don’t care about being able to select GlobalISel vs SDag separately per object file, it’d be a lot simpler to just also pass a flag to the linker, right?

I think being able to select per function would actually be a nice feature for debugging and bug reducing purposes. The real reason I tried to not go for a linker flag was because it would be an exceptional case just for GlobalISel. No other codegen option seems to rely on a linker flag (maybe I’m wrong here?). Passing it via bitcode just seemed the Right Thing To Do, even if it was a pain to implement.

If the consensus is that a one off linker flag for GlobalISel is the right way to go, then that’s ok with me.

I guess I was thinking this was an internal switch, not really expected to be set by users, so it didn’t matter much as long as you could get something that worked. But given that it is exposed as a driver option, perhaps I’m just wrong about that.

If you don’t care about being able to select GlobalISel vs SDag separately per object file, it’d be a lot simpler to just also pass a flag to the linker, right?

I think being able to select per function would actually be a nice feature for debugging and bug reducing purposes. The real reason I tried to not go for a linker flag was because it would be an exceptional case just for GlobalISel. No other codegen option seems to rely on a linker flag (maybe I’m wrong here?). Passing it via bitcode just seemed the Right Thing To Do, even if it was a pain to implement.

If the consensus is that a one off linker flag for GlobalISel is the right way to go, then that’s ok with me.

There are a few options & examples of all of them:

  • the data layout string
  • function attributes
  • mergeable (or overridable) llvm::Module metadata
  • DICompileUnit flags (sort of like the debug info equivalent of function attributes - persists through LTO/respected on a per-CU basis, etc)
  • direct MC flags with no serialization

All of these exist and are used in some cases to implement various user-facing flags. Working on debug info, I’ve seen more of the latter two than anything else. But, by way of example, compressing DWARF isn’t something LLVM currently has support for on a per-function or per-CU basis, it’s either all on, or it’s not. And at the time it was implemented we didn’t have as robust support for Module metadata - so it was implemented as a straight up MC flag. If you want to compress your debug info, you need to pass that flag to whatever command is actually the one making the native object files. (it’ll be ignored if you pass it to only the frontend compile in an LTO build - you’d have to pass it to the link step, where real object files are created)

But, yes, in /general/ there’s a goal of “LTO should work the same as non-LTO”, so flags you’d usually pass to the source → object command, should be respected even when it produces an object that’s really just an llvm::Module, etc.

That’s where function attributes and DICompileUnit flags go - so that LTO can still respect the different choices of the original input compilations. It’s admirable, but not a hard line by any means I’m aware of.

If you think it’s worth the effort to have it respected per-function, for ease of debugging and the like - yeah, it’s certainly an option!

This looks like per-function codegen pipeline. If there are existing cases of doing this, we could refer to these to implement it in an intuitive way. Overall, I think it is a bit overreaching and brings more downsides than benefits. If we do this per-TU during linking stage, could we read Module metadata and pass the flag to TargetPassConfig through TargetMachine::Options?

Yes, per TU selection sounds like a reasonable compromise. I’ll look into that.

Cheers,
Amara

This looks like per-function codegen pipeline. If there are existing cases of doing this, we could refer to these to implement it in an intuitive way. Overall, I think it is a bit overreaching and brings more downsides than benefits. If we do this per-TU during linking stage, could we read Module metadata and pass the flag to TargetPassConfig through TargetMachine::Options?

Do you mean per-original-source-TU? That amounts to per-function under LTO (because LTO will merge functions from different TUs into one Module - so then parts of a Module will do one thing, and parts will do the other).

If you want to do it on a Module-level, Module-level metadata can be used. It supports merging/override, so if two front-end compilations asked for different things (one asked for GlobalISel, one asked to not have it) you could have a rule that either fails IR linking due to the conflict, or that picks one way or the other (GlobalISel overrides if any input has picked GlobalISel).

Oh, I was thinking about ThinLTO. Right, for full LTO, there could be only one choice of GlobalISel/SDISel for all CUs if we go with Module-level metadata. Another potential option is to use llvm::splitCodeGen to split the merged module by GlobalISel/SDISel? Not sure if it is worthwhile to do so or if there are use cases to mix GlobalISel/SDISel.