Page MenuHomePhabricator

make TargetMachine visible from TargetTransformInfo
Needs ReviewPublic

Authored by vtjnash on Feb 25 2022, 12:01 PM.

Details

Reviewers
wsmoses
ychen
Summary

This replaces D118541, based on feedback there.

Out-of-tree passes sometimes need information that is not exposed by the
existing Concepts while designing new features. The frontend passes
before here and the machine passes after here already get access
directly to this, and we already expect it to be available internally
here, so it is fairly trivial to store and expose.

Diff Detail

Event Timeline

vtjnash created this revision.Feb 25 2022, 12:01 PM
vtjnash requested review of this revision.Feb 25 2022, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2022, 12:01 PM
vtjnash added a project: Restricted Project.
vtjnash added a subscriber: vchuravy.
craig.topper added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
244

Doesn't BasicTTIImpl already have access to TargetMachine via getTLI()->getTargetMachine()? See the implementation of isNoopAddrSpaceCast further down this file.

vtjnash added inline comments.Feb 25 2022, 7:22 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
244

Yes, that is why the TM value is already here: because most subtypes were already required to provide it. Are you meaning that this should be virtual? I don't think an extra pointer stored per pass manager will be a memory issue.

craig.topper added inline comments.Feb 25 2022, 9:17 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
244

I don't think I read the code very well. Ignore my comment.

It does feel like a bit of a layering violation to expose TargetMachine, which belongs to the Target library, to the Analysis layer. Analysis doesn't depend on Target. It would be considered bad practice to include TargetMachine.h in Analysis. Not sure if the same applies to forward declaration.

Echoing @craig.topper's comment, is it possible to move (or duplicate) what you need in TargetMachine to TTI?

vtjnash added a comment.EditedFeb 28 2022, 10:21 AM

TargetMachine is nearly always a required argument here, so clearly the analysis already does depend on the the TargetMachine. Sometimes the information we (aka JuliaLang) need is available in the "target-features" function attribute (which is always available), but we want to compare that against the TargetMachine's notion of the TargetFeatures during optimization. Those together are then nearly the complete set of arguments to the TargetMachine, so it seemed more logical to me to expose the TM directly, rather than separately exposing all of the information needed to reconstruct a copy of it. I don't think we should obfuscate this just for the sake of obfuscation, do you?

TargetMachine is nearly always a required argument here, so clearly the analysis already does depend on the the TargetMachine. Sometimes the information we (aka JuliaLang) need is available in the "target-features" function attribute (which is always available), but we want to compare that against the TargetMachine's notion of the TargetFeatures during optimization. Those together are then nearly the complete set of arguments to the TargetMachine, so it seemed more logical to me to expose the TM directly, rather than separately exposing all of the information needed to reconstruct a copy of it. I don't think we should obfuscate this just for the sake of obfuscation, do you?

The internals of the target implementation of TTI need TargetMachine, but it wasn't supposed to be exposed. That's why clang and opt have to create the TTI object outside of the pass manager.

Why does the TargetMachine's notion of target features matter if the "target-attribute" is present? If the "target-attribute" is present the TargetMachine features are ignored for that function.

It can matter in what ways the the "target-attribute" is different. Just grepping the source tree reveals there are several existing IR passes that depend on examining this function attribute, and are already altering their behavior based on the value. For example, it appears that the msan pass should be falling back to examining the TTI's notion of the target machine to locate the function arguments correctly, if I am reading the source comments here correctly:

https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L4220-L4232

craig.topper added a comment.EditedFeb 28 2022, 11:22 AM

It can matter in what ways the the "target-attribute" is different. Just grepping the source tree reveals there are several existing IR passes that depend on examining this function attribute, and are already altering their behavior based on the value. For example, it appears that the msan pass should be falling back to examining the TTI's notion of the target machine to locate the function arguments correctly, if I am reading the source comments here correctly:

https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L4220-L4232

When you say msan should be "falling back", do you mean when the "target-attribute" isn't present?

I agree the msan code is broken if the target-attribute isn't present. Though clang should always be emitting the attribute so it might not be an issue in practice.

I would be supportive of a patch that added a TTI function that returned the contents of the target-attribute when it is present or the TargetMachine attributes when it is not.

Hm, I am actually not 100% certain, since I think that might be a current bug now that I think about it, which was caused by failing to expose this information about the TargetMachine. It looks some passes are adding flags there (e.g. +thumb/-thumb) with the implied assumption that they should be inheriting the global TargetMachine flags for anything left unspecified, while other passes (e.g. msan) are checking only the Function's flags, with the implied assumption that the configured TargetMachine does not specify target-features, and yet other passes (MIR/Codegen) are probably assuming that the "target-features" flag should entirely replace and override the TargetMachine's feature set. These assumptions seem currently incompatible.

For example, compare the assumptions on ARM of these:
https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Transforms/IPO/LowerTypeTests.cpp#L1418-L1424
https://github.com/llvm/llvm-project/blob/d56ef5ed20c5c1380c2d97e970c8fc4d4166bdb8/llvm/lib/Target/ARM/ARMTargetMachine.cpp#L268-L273

I am not trying to solve this here, but merely provide access to the tools (TargetMachine cpu architecture and features) so that these passes–and others like them–can be fixed.

I would be supportive of a patch that added a TTI function that returned the contents of the target-attribute when it is present or the TargetMachine attributes when it is not.

It appear that if we add that function, then there only be one missing argument (CPU) that would be needed so that any user could re-allocate their own private copy of the TargetMachine (see the constructor at https://llvm.org/doxygen/classllvm_1_1TargetMachine.html). Is there some reason it is better to expose each of the values separately, but not the object that they come from?

Alternative observation to merging this PR: since the Module object already must contain the first 3 arguments to the TargetMachine constructor, would it be more logical to attach the TM directly to the module instead, replacing separate values, instead of needing the fake analysis passes like MachineModuleAnalysis to expose it to CodeGen?

Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 1:18 PM