[PATCH] D123198: [LibCalls] Add argument extension attributes to more functions.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/BuildLibCalls.h:38
+  FunctionCallee getOrInsertLibFunc(Module *M, const TargetLibraryInfo &TLI,
+                                LibFunc TheLibFunc,  StringRef Name,
+                                FunctionType *T, AttributeList AttributeList);
----------------
Does getOrInsertLibFunc really need a "Name" argument?  We have TargetLibraryInfo; we can just look up the correct name.


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1238
+  // Make sure that any narrow integer argument is extended if required by
+  // the target ABI.
+  if (TLI.getExtAttrForI32Param() != Attribute::None)
----------------
I'd prefer to make the assertion target-independent if possible; if someone is adding a new libcall optimization, they might not remember to write a test specifically for SystemZ.

Can we just add a check in the "default" case that the call doesn't pass/return any integer values, or something like that?  (I guess in that case, you'd have to list out all the calls we build that have a size_t argument or return value, but that should be a much smaller list than every function supported by inferNonMandatoryLibFuncAttrs.)


================
Comment at: llvm/test/Transforms/InstCombine/strchr-1.ll:3
 ; Test that the strchr library call simplifier works correctly.
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+; RUN: opt < %s -passes=instcombine -S -mtriple=systemz-unknown | FileCheck %s -check-prefixes=CHECK-SYSTEMZ
----------------
jonpa wrote:
> Will this test pass on all targets or should the first line here have a target triple to match the output (uses of i64)...?
> Or perhaps running it just once on SystemZ would work?
> 
> (fputs-1.ll same)
> 
> 
In general, if you're going to manually hack at tests with an "Assertions have been autogenerated" marking, please either regenerate them or drop the marking.

These tests were written with the assumption the generated IR wouldn't be target-specific.  Since it turns out the input and result are in fact target-specific, we should just pick a target, and shove them into the relevant target-specific subdirectory (llvm/test/Transforms/InstCombine/X86/ etc.).  And maybe add a few tests to llvm/test/Transforms/InstCombine/SystemZ/ to confirm the whole signext marking mechanism is working.

Alternatively, you could try to hack at the tests to try make them agnostic to whether there's a signext marking, but that seems like extra work for not much benefit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123198/new/

https://reviews.llvm.org/D123198