[PATCH] D22868: [AArch64] Register passes so they can be run by llc

Diana Picus via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 28 00:54:00 PDT 2016


rovka added a comment.

In https://reviews.llvm.org/D22868#497831, @t.p.northover wrote:

> Why are only some names conflicting? I'm not quite sure why registering a pass would add an option, at least not in all cases (it seems particularly inappropriate for llc, which doesn't work like that).


Right, I wasn't very clear about this - the conflicts are in opt. I'm not particularly excited about the fact that llc and opt work in different ways, nor about the fact that you need to do *some stuff* to see your pass in print-before-all and *some other stuff* so you can use print-before=BlahPass, but I guess that's not the point here.
You only get conflicts in some cases because not all registered passes have an option defined in AArch64TargetMachine [1], or the options have a different name than the one the passes are registered with (e.g. "aarch64-stp-suppress" vs "aarch64-store-pair-suppress").

> It would also be good to be consistent if possible rather than having some with "-aarch64-enable-whatever" and some just "-aarch64-whatever".


Now that you mention it, I guess I could get creative about the names we use to register the passes, and then we'd avoid the conflicts in the first place. Alternatively, we could be a bit more tidy and rename all the options to "-aarch64-enable-whatever", because that describes their effect better. Do you have any preference here?

Another inconsistency here is *where* the options are defined - a bunch of them are in AArch64TargetMachine, but that's not the case for all of the passes, e.g. AArch64BranchRelaxation defines the option in its own file. I'm slightly in favor of keeping the option in the same file as the pass. I'm volunteering to move all of them if you think it's a good idea.

Thanks for having a look.

[1] e.g. AFAICT the local-dynamic TLS cleanup pass doesn't have an option; although there is a flag that sounds like it in AArch64ISelLowering, it doesn't seem to affect it


https://reviews.llvm.org/D22868





More information about the llvm-commits mailing list