Page MenuHomePhabricator

[strictfp] Replace dangling strictfp attrs with nobuiltin
ClosedPublic

Authored by kpn on Nov 11 2019, 11:45 AM.

Details

Summary

In preparation for a patch that will enforce new rules for the usage of the strictfp attribute, this patch introduces auto-upgrade behavior that
will replace the strictfp attribute on callsites with nobuiltin if the enclosing function declaration doesn't also have the strictfp attribute.

This auto-upgrade isn't being performed on .ll files because that would prevent us from writing a test for the forthcoming verifier behavior.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2019, 11:45 AM

@pcc I've added you as a reviewer in hopes that you can tell me whether my use of code in globalCleanup() is correct. It seems to be working, but Craig was concerned that there might be cases under which this would be called when things weren't sufficiently materialized to properly handle the distinction between function declaration and definition.

kpn added a comment.Nov 11 2019, 12:08 PM

Do we want to restrict this so it doesn't get upgraded if found in a future bitcode version?

Do we want to not upgrade if a constrained intrinsic is used anywhere in the function? We do want to be able to distinguish between pre-constrained intrinsic uses and modern constrained uses that are wrong. I am assuming that the upgrade happens before the IR verifier runs.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
2992

typo: s/function/functions/

In D70096#1741213, @kpn wrote:

Do we want to restrict this so it doesn't get upgraded if found in a future bitcode version?

How would we do that?

Do we want to not upgrade if a constrained intrinsic is used anywhere in the function? We do want to be able to distinguish between pre-constrained intrinsic uses and modern constrained uses that are wrong. I am assuming that the upgrade happens before the IR verifier runs.

Honestly, I'd be very surprised if anyone is using this in a product currently. After the verifier support is added we'll be able to catch incorrect uses as they happen. Incorrect shouldn't get to this point. After the verifier support is complete, functions containing constrained intrinsics without the strictfp attribute on the function definition will fail verification, so I don't think checking for that case here really adds anything.

I'm also assuming that the upgrade happens before the verifier runs. I've chosen to only add this auto-upgrade in the BitcodeReader so that we can still write tests in the text format when the verifier support is added.

kpn added a comment.Nov 12 2019, 12:09 PM
In D70096#1741213, @kpn wrote:

Do we want to restrict this so it doesn't get upgraded if found in a future bitcode version?

How would we do that?

BitcodeReaderBase knows the version number but it doesn't store it anywhere. It does use it to set UseStrtab, but that's it.

Thinking about it more, this incorrect bitcode case can only happen going forward if someone had a different bitcode writer implementation, or at least an llvm one that was hacked to allow this strictfp error. That seems like a stretch. So maybe it doesn't matter.

Do we want to not upgrade if a constrained intrinsic is used anywhere in the function? We do want to be able to distinguish between pre-constrained intrinsic uses and modern constrained uses that are wrong. I am assuming that the upgrade happens before the IR verifier runs.

Honestly, I'd be very surprised if anyone is using this in a product currently.

If nobody is using it then errors are pretty moot. When I asked for input from front-end guys of any language about strictfp changes for the IRBuilder I only got "don't care" responses because nobody who responded wanted to use strictfp. So it seems safe to just assume you are correct here.

kpn added a comment.Nov 13 2019, 7:57 AM

Does strictfp need to be on function declarations? Currently it isn't.

kpn added a comment.Nov 14 2019, 8:02 AM
In D70096#1744035, @kpn wrote:

Does strictfp need to be on function declarations? Currently it isn't.

On second thought, no, I don't think we need this. So, um, nevermind.

Rebase and fix typo

mehdi_amini added inline comments.
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
5361

I think you need to call the new upgrade function from here as well.

llvm/test/Bitcode/compatibility-5.0.ll
1256

It'd be nice to have a test that check that the callsite attribute isn't changed when used in a non-strictfp function.

andrew.w.kaylor marked 2 inline comments as done.Jan 30 2020, 3:09 PM

Thanks for the review!

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
5361

OK, thanks. I wasn't clear how the materialize stuff works. Is there some different code path that would get me here? My test is passing with the code as is, so does that mean I need an additional test?

llvm/test/Bitcode/compatibility-5.0.ll
1256

In older versions of the IR, the strictfp attribute was only used on callsites (at least by clang). Does that matter? Do I need to figure out which version started adding it to function definitions? I think it was always legal since the attribute was introduced. It just had no effect until recently.

kpn added a comment.Apr 1 2020, 2:45 PM

I ran a check myself. I found that there are quite a few existing tests that enter BitcodeReader::materialize() from someplace other than BitcodeReader::materializeModule(). Perhaps one of these would be helpful in coming up with a new test?

LLVM-Unit :: Bitcode/./BitcodeTests/BitReaderTest.MaterializeFunctionsForBlockAddr
LLVM-Unit :: Bitcode/./BitcodeTests/BitReaderTest.MaterializeFunctionsForBlockAddrInFunctionAfter
LLVM-Unit :: Bitcode/./BitcodeTests/BitReaderTest.MaterializeFunctionsForBlockAddrInFunctionBefore
LLVM-Unit :: Bitcode/./BitcodeTests/BitReaderTest.MaterializeFunctionsOutOfOrder
LLVM :: Analysis/StackSafetyAnalysis/ipa-alias.ll
LLVM :: Feature/terminators.ll
LLVM :: LTO/ARM/lto-linking-metadata.ll
LLVM :: LTO/Resolution/X86/appending-var.ll
LLVM :: LTO/Resolution/X86/asm-output.ll
LLVM :: LTO/Resolution/X86/comdat-mixed-lto.ll
LLVM :: LTO/Resolution/X86/comdat.ll
LLVM :: LTO/Resolution/X86/common2.ll
LLVM :: LTO/Resolution/X86/dead-strip-fulllto.ll
LLVM :: LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll
LLVM :: LTO/Resolution/X86/diagnostic-handler-remarks.ll
LLVM :: LTO/Resolution/X86/function-alias-non-prevailing.ll
LLVM :: LTO/Resolution/X86/ifunc2.ll
LLVM :: LTO/Resolution/X86/link-odr-availextern.ll
LLVM :: LTO/Resolution/X86/linker-redef.ll
LLVM :: LTO/Resolution/X86/load-sample-prof-lto.ll
LLVM :: LTO/Resolution/X86/mixed_lto.ll
LLVM :: LTO/Resolution/X86/setting-dso-local.ll
LLVM :: LTO/Resolution/X86/type-checked-load.ll
LLVM :: LTO/X86/codemodel-1.ll
LLVM :: LTO/X86/codemodel-2.ll
LLVM :: LTO/X86/codemodel-3.ll
LLVM :: LTO/X86/dllimport.ll
LLVM :: LTO/X86/embed-bitcode.ll
LLVM :: LTO/X86/internalize.ll
LLVM :: LTO/X86/pr38046.ll
LLVM :: LTO/X86/strip-debug-info.ll
LLVM :: LTO/X86/symver-asm.ll
LLVM :: LTO/X86/tailcallelim.ll
LLVM :: LTO/X86/type-mapping-bug2.ll
LLVM :: LTO/X86/type-mapping-bug3.ll
LLVM :: Linker/2003-01-30-LinkerRename.ll
LLVM :: Linker/2003-04-26-NullPtrLinkProblem.ll
LLVM :: Linker/2003-08-28-TypeResolvesGlobal.ll
LLVM :: Linker/2003-08-28-TypeResolvesGlobal2.ll
LLVM :: Linker/2003-08-28-TypeResolvesGlobal3.ll
LLVM :: Linker/2003-11-18-TypeResolution.ll
LLVM :: Linker/2004-05-07-TypeResolution1.ll
LLVM :: Linker/2006-01-19-ConstantPacked.ll
LLVM :: Linker/2008-03-05-AliasReference.ll
LLVM :: Linker/2008-07-06-AliasFnDecl.ll
LLVM :: Linker/2008-07-06-AliasWeakDest.ll
LLVM :: Linker/2009-09-03-mdnode.ll
LLVM :: Linker/AppendingLinkage.ll
LLVM :: Linker/basiclink.ll
LLVM :: Linker/drop-debug.ll
LLVM :: Linker/funcimport.ll
LLVM :: Linker/funcimport2.ll
LLVM :: Linker/funcimport_appending_global.ll
LLVM :: Linker/funcimport_comdat.ll
LLVM :: Linker/link-arm-and-thumb.ll
LLVM :: Linker/link-global-to-func.ll
LLVM :: Linker/linkage.ll
LLVM :: Linker/linkmdnode.ll
LLVM :: Linker/only-needed-debug-metadata.ll
LLVM :: Linker/pr21374.ll
LLVM :: Linker/pr26037.ll
LLVM :: Linker/thinlto_funcimport_debug.ll
LLVM :: Linker/transitive-lazy-link.ll
LLVM :: Linker/type-unique-src-type.ll
LLVM :: Other/extract.ll
LLVM :: ThinLTO/X86/alias_import.ll
LLVM :: ThinLTO/X86/autoupgrade.ll
LLVM :: ThinLTO/X86/builtin-nostrip.ll
LLVM :: ThinLTO/X86/cache-import-lists.ll
LLVM :: ThinLTO/X86/cache-typeid-resolutions.ll
LLVM :: ThinLTO/X86/cache.ll
LLVM :: ThinLTO/X86/callees-metadata.ll
LLVM :: ThinLTO/X86/crash_debuginfo.ll
LLVM :: ThinLTO/X86/deadstrip.ll
LLVM :: ThinLTO/X86/debuginfo-compositetype-import.ll
LLVM :: ThinLTO/X86/debuginfo-cu-import.ll
LLVM :: ThinLTO/X86/devirt.ll
LLVM :: ThinLTO/X86/devirt2.ll
LLVM :: ThinLTO/X86/devirt_single_hybrid.ll
LLVM :: ThinLTO/X86/devirt_vcall_vis_hidden.ll
LLVM :: ThinLTO/X86/devirt_vcall_vis_public.ll
LLVM :: ThinLTO/X86/diagnostic-handler-remarks-with-hotness.ll
LLVM :: ThinLTO/X86/diagnostic-handler-remarks.ll
LLVM :: ThinLTO/X86/dicompositetype-unique-alias.ll
LLVM :: ThinLTO/X86/dicompositetype-unique.ll
LLVM :: ThinLTO/X86/dicompositetype-unique2.ll
LLVM :: ThinLTO/X86/distributed_import.ll
LLVM :: ThinLTO/X86/dot-dumper-full-lto.ll
LLVM :: ThinLTO/X86/dot-dumper.ll
LLVM :: ThinLTO/X86/dot-dumper2.ll
LLVM :: ThinLTO/X86/drop-debug-info.ll
LLVM :: ThinLTO/X86/funcimport-debug.ll
LLVM :: ThinLTO/X86/funcimport-stats.ll
LLVM :: ThinLTO/X86/funcimport-tbaa.ll
LLVM :: ThinLTO/X86/funcimport.ll
LLVM :: ThinLTO/X86/funcimport2.ll
LLVM :: ThinLTO/X86/funcimport_alwaysinline.ll
LLVM :: ThinLTO/X86/function_entry_count.ll
LLVM :: ThinLTO/X86/globals-import-blockaddr.ll
LLVM :: ThinLTO/X86/globals-import-const-fold.ll
LLVM :: ThinLTO/X86/globals-import.ll
LLVM :: ThinLTO/X86/guid_collision.ll
LLVM :: ThinLTO/X86/import-constant.ll
LLVM :: ThinLTO/X86/import-ro-constant.ll
LLVM :: ThinLTO/X86/import_opaque_type.ll
LLVM :: ThinLTO/X86/index-const-prop-alias.ll
LLVM :: ThinLTO/X86/index-const-prop-cache.ll
LLVM :: ThinLTO/X86/index-const-prop-comdat.ll
LLVM :: ThinLTO/X86/index-const-prop-full-lto.ll
LLVM :: ThinLTO/X86/index-const-prop-gvref.ll
LLVM :: ThinLTO/X86/index-const-prop-ldst.ll
LLVM :: ThinLTO/X86/index-const-prop-linkage.ll
LLVM :: ThinLTO/X86/index-const-prop.ll
LLVM :: ThinLTO/X86/index-const-prop2.ll
LLVM :: ThinLTO/X86/lazyload_metadata.ll
LLVM :: ThinLTO/X86/linkonce_aliasee_ref_import.ll
LLVM :: ThinLTO/X86/llvm.used.ll
LLVM :: ThinLTO/X86/local_name_conflict.ll
LLVM :: ThinLTO/X86/local_name_conflict_var.ll
LLVM :: ThinLTO/X86/module_asm2.ll
LLVM :: ThinLTO/X86/module_asm_glob.ll
LLVM :: ThinLTO/X86/personality-local.ll
LLVM :: ThinLTO/X86/personality.ll
LLVM :: ThinLTO/X86/pr35472.ll
LLVM :: ThinLTO/X86/printer.ll
LLVM :: ThinLTO/X86/reference_non_importable.ll
LLVM :: ThinLTO/X86/referenced_by_constant.ll
LLVM :: ThinLTO/X86/save_objects.ll
LLVM :: ThinLTO/X86/section.ll
LLVM :: ThinLTO/X86/writeonly-with-refs.ll
LLVM :: ThinLTO/X86/writeonly.ll
LLVM :: ThinLTO/X86/writeonly2.ll
LLVM :: Transforms/FunctionImport/adjustable_threshold.ll
LLVM :: Transforms/FunctionImport/comdat.ll
LLVM :: Transforms/FunctionImport/funcimport.ll
LLVM :: Transforms/FunctionImport/funcimport_alias.ll
LLVM :: Transforms/FunctionImport/funcimport_cutoff.ll
LLVM :: Transforms/FunctionImport/funcimport_debug.ll
LLVM :: Transforms/FunctionImport/funcimport_forcecold.ll
LLVM :: Transforms/FunctionImport/funcimport_forcecold_samplepgo.ll
LLVM :: Transforms/FunctionImport/funcimport_resolved.ll
LLVM :: Transforms/FunctionImport/hotness_based_import.ll
LLVM :: Transforms/FunctionImport/import_stats.ll
LLVM :: Transforms/PGOProfile/thinlto_cspgo_gen.ll
LLVM :: Transforms/PGOProfile/thinlto_cspgo_use.ll
LLVM :: Transforms/PGOProfile/thinlto_indirect_call_promotion.ll
LLVM :: Transforms/PGOProfile/thinlto_samplepgo_icp.ll
LLVM :: Transforms/PGOProfile/thinlto_samplepgo_icp2.ll
LLVM :: Transforms/PGOProfile/thinlto_samplepgo_icp3.ll
LLVM :: tools/llvm-lto2/X86/pipeline.ll
LLVM :: tools/llvm-lto2/X86/stats-file-option.ll
LLVM :: tools/llvm-split/blockaddress.ll

I believe I removed from the list the tests that always fail for me because I use NFS pretty much always. I may have missed one or two.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
5361

Yes, it looks like {Thin,}LTO is relevant plus others. See the attached list.

kpn commandeered this revision.May 15 2020, 7:53 AM
kpn edited reviewers, added: andrew.w.kaylor; removed: kpn.

Commandeering with Andy's permission.

kpn updated this revision to Diff 264246.May 15 2020, 7:54 AM

Address review comments, and add a test to verify the additional code.

kpn updated this revision to Diff 266916.May 28 2020, 9:24 AM

Update to not "upgrade" the strictfp attribute on a call to a constrained FP intrinsic. When my IR verifier changes land it will result in a misleading error message.

This revision is now accepted and ready to land.Fri, Jun 12, 11:39 AM
This revision was automatically updated to reflect the committed changes.