Page MenuHomePhabricator

[Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values
ClosedPublic

Authored by skc7 on Jul 21 2022, 12:10 AM.

Details

Summary

Add the ability to put attribute((maybe_undef)) on function arguments. Clang will now introduce a freeze instruction on the argument. Previous review relating to the noundef attribute issue: D128907.

llvm discourse RFC topic: RFC

Diff Detail

Event Timeline

skc7 created this revision.Jul 21 2022, 12:10 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: jdoerfert. · View Herald Transcript
skc7 requested review of this revision.Jul 21 2022, 12:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 12:10 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
skc7 edited the summary of this revision. (Show Details)Jul 21 2022, 12:17 AM
skc7 edited the summary of this revision. (Show Details)Jul 21 2022, 4:49 AM

I'm in C standards meetings this week and don't have a lot of ability to thoroughly review this yet, but the most immediate concern which springs to mind for me is that this is exposing LLVM implementation details to users. Users should not have to think about things in terms of LLVM IR markings like poison and undef, and I worry that this is an expert-only feature that will be easy to misuse and introduce security issues. It sounds like there's an extremely specific use case in mind for this attribute; is there a reason why this needs to be the user's problem instead of something the compiler can infer or handle on the backend?

It sounds like there's an extremely specific use case in mind for this attribute; is there a reason why this needs to be the user's problem instead of something the compiler can infer or handle on the backend?

The intended user is a handful of header wrapper functions around intrinsics, not general users

It sounds like there's an extremely specific use case in mind for this attribute; is there a reason why this needs to be the user's problem instead of something the compiler can infer or handle on the backend?

The intended user is a handful of header wrapper functions around intrinsics, not general users

Great, then it sounds like we don't need to expose this as an attribute because we know which intrinsics need special behavior, so we can handle them specially at codegen time or within the backend?

It sounds like there's an extremely specific use case in mind for this attribute; is there a reason why this needs to be the user's problem instead of something the compiler can infer or handle on the backend?

The intended user is a handful of header wrapper functions around intrinsics, not general users

Great, then it sounds like we don't need to expose this as an attribute because we know which intrinsics need special behavior, so we can handle them specially at codegen time or within the backend?

The problem is the wrappers. We originally talked about specifically handling the builtins, but that falls apart given the real users come from wrapper functions which look like ordinary code

skc7 updated this revision to Diff 446727.Jul 21 2022, 11:57 PM
skc7 edited the summary of this revision. (Show Details)

Rebase. Remove skipping noundef attribute based on maybe_undef.

I'm in C standards meetings this week and don't have a lot of ability to thoroughly review this yet, but the most immediate concern which springs to mind for me is that this is exposing LLVM implementation details to users. Users should not have to think about things in terms of LLVM IR markings like poison and undef, and I worry that this is an expert-only feature that will be easy to misuse and introduce security issues.

Here's how I would tentatively describe the attribute in terms that mesh better with how I understand C and C++:

As an exception to the rule that loading from an unitialized variable is undefined behavior, if the loaded value is used immediately as an __attribute__((maybe_undef)) argument in a function call, the loaded value is implementation-defined. It may vary between multiple runs of the program, and it may vary between multiple uses of the uninitialized variable.

This requires no thinking about LLVM IR and undef/poison.

There may have to be some caveats about bools and enums (generally, types where not all possible values in memory are actually legal values of the type), but I don't know enough about those language standards to judge that.

clang/include/clang/Basic/AttrDocs.td
276

param shouldn't be of void type, right?

Generating freeze seems an option, just not sure we can easily get all locations with this approach.
Anyway, some code comments below.

clang/lib/CodeGen/CGCall.cpp
2061

Early exists please.

if (!TargetDecl)

return false

no ArgHasMayBeUndefAttr is needed, just return true/false, etc.

4910

Style: Val, also below.

clang/test/CodeGenHIP/maybe_undef-attr-verify.hip
34

Note, not necessarily to address: I would have preferred auto generated check lines, should not be many and it's easier to see what's going on, IMHO.

skc7 updated this revision to Diff 446919.Jul 22 2022, 11:54 AM

Fixes as per review comments.

skc7 updated this revision to Diff 447383.Jul 25 2022, 9:51 AM
skc7 edited the summary of this revision. (Show Details)

Rebase. Ping.

It sounds like there's an extremely specific use case in mind for this attribute; is there a reason why this needs to be the user's problem instead of something the compiler can infer or handle on the backend?

The intended user is a handful of header wrapper functions around intrinsics, not general users

Great, then it sounds like we don't need to expose this as an attribute because we know which intrinsics need special behavior, so we can handle them specially at codegen time or within the backend?

The problem is the wrappers. We originally talked about specifically handling the builtins, but that falls apart given the real users come from wrapper functions which look like ordinary code

I'm still not seeing the issue fully. My understanding of the situation (which may be wrong) is that Clang lowers to LLVM IR and adds noundef markings at call sites that this patch attempts to let the user then undo. However, why can Clang's CodeGen not notice the special builtin and walk up the call chain to mark all the wrapper functions to ensure everything is marked appropriately? There might be a small perf loss (if there are wrappers around wrappers around wrappers kind of situation), but this means we don't have to expose this problem to users.

I'm in C standards meetings this week and don't have a lot of ability to thoroughly review this yet, but the most immediate concern which springs to mind for me is that this is exposing LLVM implementation details to users. Users should not have to think about things in terms of LLVM IR markings like poison and undef, and I worry that this is an expert-only feature that will be easy to misuse and introduce security issues.

Here's how I would tentatively describe the attribute in terms that mesh better with how I understand C and C++:

As an exception to the rule that loading from an unitialized variable is undefined behavior, if the loaded value is used immediately as an __attribute__((maybe_undef)) argument in a function call, the loaded value is implementation-defined. It may vary between multiple runs of the program, and it may vary between multiple uses of the uninitialized variable.

This requires no thinking about LLVM IR and undef/poison.

Thanks, this helps somewhat (though I would argue that the loaded value is *not* implementation-defined but is instead unspecified, because that matches the semantics you mention about not needing to pick the same loaded value every time). However, this design is basically asking the user to decide when something should or should not be undefined behavior, which means this attribute will almost certainly be abused under the guise of "this makes my program safer because it removes UB". I still think that a solution which doesn't require the user to make such dangerous decisions on their own is a better design if we can accomplish it.

I'm still not seeing the issue fully. My understanding of the situation (which may be wrong) is that Clang lowers to LLVM IR and adds noundef markings at call sites that this patch attempts to let the user then undo. However, why can Clang's CodeGen not notice the special builtin and walk up the call chain to mark all the wrapper functions to ensure everything is marked appropriately? There might be a small perf loss (if there are wrappers around wrappers around wrappers kind of situation), but this means we don't have to expose this problem to users.

This requires maintaining logic in clang to specially recognize these N wrapper functions, and there would be no indication in the source that this magic is occurring. It's less maintainable because it requires tight, magic coupling between these functions in the source and special handling in clang to be maintained if anything were to change

I'm still not seeing the issue fully. My understanding of the situation (which may be wrong) is that Clang lowers to LLVM IR and adds noundef markings at call sites that this patch attempts to let the user then undo. However, why can Clang's CodeGen not notice the special builtin and walk up the call chain to mark all the wrapper functions to ensure everything is marked appropriately? There might be a small perf loss (if there are wrappers around wrappers around wrappers kind of situation), but this means we don't have to expose this problem to users.

I think you are right, we could identify shuffles and go up the call chain in clang (codegen) if we wanted.

I still think that a solution which doesn't require the user to make such dangerous decisions on their own is a better design if we can accomplish it.

This is a valid opinion but I don't see how this matches what we do (or has been done) in the past. I mean, there are plenty of command line options and attributes that do exactly this, let the user "define", or better "refine", semantics.
The entire "initialize with 0" stuff comes to mind, making new things UB with -ffast-math and friends, overloadable, etc.


All that said, I'm generally more in favor of explicit annotations in the source than implicit compiler magic to implement a special case.
Others should chime in.

I'm still not seeing the issue fully. My understanding of the situation (which may be wrong) is that Clang lowers to LLVM IR and adds noundef markings at call sites that this patch attempts to let the user then undo. However, why can Clang's CodeGen not notice the special builtin and walk up the call chain to mark all the wrapper functions to ensure everything is marked appropriately? There might be a small perf loss (if there are wrappers around wrappers around wrappers kind of situation), but this means we don't have to expose this problem to users.

This requires maintaining logic in clang to specially recognize these N wrapper functions, and there would be no indication in the source that this magic is occurring. It's less maintainable because it requires tight, magic coupling between these functions in the source and special handling in clang to be maintained if anything were to change

I was thinking of something more general than that where Clang only has to specially recognize the builtins and which arguments to the builtin should be tracked to make maybeundef when walking up the call chain. However, you're not wrong about this being a bit magical in terms of behavior. I don't know that we have anything else which behaves this way.

I still think that a solution which doesn't require the user to make such dangerous decisions on their own is a better design if we can accomplish it.

This is a valid opinion but I don't see how this matches what we do (or has been done) in the past. I mean, there are plenty of command line options and attributes that do exactly this, let the user "define", or better "refine", semantics.
The entire "initialize with 0" stuff comes to mind, making new things UB with -ffast-math and friends, overloadable, etc.

Thanks, this is a good question to be asking!

We are adding attributes in a way which doesn't scale well; many attributes interact with one another and we have basically no test coverage for those situations and very little thought put into diagnostic behavior. Now that we've got over 400 attributes in the tree, I'm realizing I probably should have been saying "no" more to attributes that don't carry enough weight to be worth the maintenance burden, but oh well, I can't unring that bell. I have *no issue* with attributes that are going to be things we expect users to use themselves. Stuff like "initialize with 0", overloadable, etc are all things that enable users to do something they couldn't previously do. However, in recent history, we've been adding more attributes that are for compiler glue code rather than users. On the one hand, attributes are a great use for such a thing. On the other hand, there's no way to control who uses the attribute (unless we put in special logic to only allow it to be written in a system header, or something like that). So glue code attributes are a different "thing" to me than user-facing attributes, and I'm worried at how many we're adding without any real design thought put behind them other than "we need it to solve this one case". If we could come up with some tablegen marking to say "this attribute is for glue code use only" that we could use to generate diagnostics on "misuse", generate documentation differently from, etc, that would go a long ways towards alleviating the concerns about user confusion with glue code attributes and "real" attributes (for lack of a better term).

Of course, these are high-level worries and not necessarily specific to this review or anything I expect someone here to take action on. Take this as background information rather than a code owner mandate or anything like that. Specific to this review, my concern is that this is an attribute that controls undefined behavior in LLVM rather directly (as opposed to things like "initialize to 0" which impact UB, but aren't as tightly tied to the LLVM backend architecture). That's a worry for two reasons: the tight coupling to LLVM IR attribute semantics (basically, this means LLVM's "freeze" instruction can't change meaning because it will potentially break user code) and misuse due to misunderstanding the purpose of the attribute ("this makes my code more secure!"). Special compiler magic alleviates both of those concerns because it means we aren't exposing the tight coupling and users aren't given a chance to misuse the attribute.

However, what I think I'm hearing from this thread is that there are alternative approaches that have been thought about but not tried, we're not certain how feasible those approaches are in practice, but we expect them to be materially worse than what's proposed here. So it's not "this was the path of least resistance", but "this is the correct design." Do others agree with that assessment?

However, what I think I'm hearing from this thread is that there are alternative approaches that have been thought about but not tried, we're not certain how feasible those approaches are in practice, but we expect them to be materially worse than what's proposed here. So it's not "this was the path of least resistance", but "this is the correct design." Do others agree with that assessment?

I think this is just the least bad on the menu of available options. I don't like it, but it at least provides documentation about this special behavior.

I think the only other plausible option is to assert this is still undefined behavior and force users to update their (newly declared invalid) code. We could at least partially re-optimize to uninitialized values in the backend (although this is apparently difficult in some situations)

However, what I think I'm hearing from this thread is that there are alternative approaches that have been thought about but not tried, we're not certain how feasible those approaches are in practice, but we expect them to be materially worse than what's proposed here. So it's not "this was the path of least resistance", but "this is the correct design." Do others agree with that assessment?

I think this is just the least bad on the menu of available options. I don't like it, but it at least provides documentation about this special behavior.

I think the only other plausible option is to assert this is still undefined behavior and force users to update their (newly declared invalid) code. We could at least partially re-optimize to uninitialized values in the backend (although this is apparently difficult in some situations)

First off, thank you for the great discussion about this both on and off lists while I wrapped my head around the design space and challenges involved. I appreciate your willingness to explore alternatives with me!

I think I agree that this is the least bad option on the menu. The code the user wrote is undefined behavior, and the CUDA/HIP standards would like to pretend otherwise in some circumstances. Given that users will have code invalidated if we stick to the hard line language rules, I think it's better than we remove the UB than punish the user and hope they can write a more convoluted but correct form.

In terms of the patch itself, I've left some comments. But this is also missing all semantic tests (that the attribute is diagnosed when written on something other than a parameter, accept no arguments, etc). It also is missing tests for some interesting use cases:

void func(int param);

void other() {
  int derp;
  func(derp); // How should the call behave?
}

void func(__attribute__((maybe_undef)) int param) { ... }

// Also:
void foo(__attribute__((maybe_undef)) int param);
void foo(int param) { ... } // Verify this parameter behaves as expected

void bar() {
  int derp;
  foo(derp);
}

It'd probably make sense to have a case involving templates as well to ensure that instantiation properly retains the parameter attribute, but we might already have test coverage for that (parameter attributes are pretty uncommon, but not unheard of).

clang/include/clang/Basic/Attr.td
2026
clang/include/clang/Basic/AttrDocs.td
260
261

I think this should be DocCatVariable temporarily, but we probably should add a new DocCatParameter category at some point and use that instead (in case you're feeling ambitious).

263
265
clang/lib/CodeGen/CGCall.cpp
2050–2066

One question I have is whether you ever need to mark the variadic arguments as being maybe undef. e.g., void func(int i, ...); do you need to signal that arguments passed to ... are maybe undef?

skc7 updated this revision to Diff 447993.Jul 27 2022, 3:57 AM

Changes for code review comments from @aaron.ballman

skc7 added inline comments.Jul 27 2022, 4:05 AM
clang/lib/CodeGen/CGCall.cpp
2050–2066

Current change assumes variadic arguments will not have "maybe_undef" attribute. If its a function attribute, variadic arguments can inherit them (Have seen such cases in clang codebase). But "maybe_undef" is function argument attribute and I'm not sure on how to add it to variadic arguments.

aaron.ballman added inline comments.Jul 27 2022, 4:14 AM
clang/lib/CodeGen/CGCall.cpp
2050–2066

Current change assumes variadic arguments will not have "maybe_undef" attribute. If its a function attribute, variadic arguments can inherit them (Have seen such cases in clang codebase). But "maybe_undef" is function argument attribute and I'm not sure on how to add it to variadic arguments.

That's why I was asking if there will be a need either now or in the future -- is making this a parameter attribute the right design, or should this be a function attribute with positional arguments? (Not that I particularly love that design, but this isn't a user-facing attribute so if it's hard to use, I'm not that worried. I just want to make sure we don't need to add additional attributes in the future in this same space.)

I can't see a scenario where we would need this with a variadic call. Ultimately these wrap specific physical instructions, not some kind of arbitrary call

FWIW, precommit CI has some relevant failures for a change:

******************** TEST 'Clang :: CodeGen/attr-maybeundef-template.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -no-opaque-pointers -emit-llvm C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp -o - | c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe" "-cc1" "-internal-isystem" "c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include" "-nostdsysteminc" "-no-opaque-pointers" "-emit-llvm" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp" "-o" "-"
$ "c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp"
# command stderr:
C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp:3:11: error: CHECK: expected string not found in input
// CHECK: define weak_odr void @_Z5test4IfEvT_(float noundef [[TMP1:%.*]])
          ^
<stdin>:1:1: note: scanning from here
; ModuleID = 'C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp'
^
<stdin>:11:14: note: possible intended match here
define weak_odr dso_local void @"??$test4@M@@YAXM@Z"(float noundef %arg) #0 comdat {
             ^

Input file: <stdin>
Check file: C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
           1: ; ModuleID = 'C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp' 
check:3'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
           2: source_filename = "C:\\ws\\w4\\llvm-project\\premerge-checks\\clang\\test\\CodeGen\\attr-maybeundef-template.cpp" 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           3: target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           4: target triple = "x86_64-pc-windows-msvc" 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           5:  
check:3'0     ~
           6: $"??$test4@M@@YAXM@Z" = comdat any 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           7:  
check:3'0     ~
           8: $"??$test4@H@@YAXH@Z" = comdat any 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           9:  
check:3'0     ~
          10: ; Function Attrs: mustprogress noinline nounwind optnone 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          11: define weak_odr dso_local void @"??$test4@M@@YAXM@Z"(float noundef %arg) #0 comdat { 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:3'1                  ?                                                                        possible intended match
          12: entry: 
check:3'0     ~~~~~~~
          13:  %arg.addr = alloca float, align 4 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          14:  store float %arg, float* %arg.addr, align 4 
check:3'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          15:  ret void 
check:3'0     ~~~~~~~~~~
          16: } 
check:3'0     ~~
           .
           .
           .
>>>>>>

error: command failed with exit status: 1

--

********************
FAIL: Clang :: CodeGen/attr-maybeundef.c (4048 of 15750)
******************** TEST 'Clang :: CodeGen/attr-maybeundef.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -no-opaque-pointers -emit-llvm C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c -o - | c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe" "-cc1" "-internal-isystem" "c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include" "-nostdsysteminc" "-no-opaque-pointers" "-emit-llvm" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c" "-o" "-"
# command stderr:
C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c:33:6: warning: 'maybe_undef' attribute only applies to parameters [-Wignored-attributes]

void __maybe_undef t2(int param1, int param2, float param3) {

     ^

C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c:3:38: note: expanded from macro '__maybe_undef'

#define __maybe_undef __attribute__((maybe_undef))

                                     ^

1 warning generated.


$ "c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c"
# command stderr:
C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c:7:16: error: CHECK-NEXT: is not on the line after the previous match
// CHECK-NEXT: [[TMP4:%.*]] = alloca i32, align 4
               ^
<stdin>:10:2: note: 'next' match was here
 %param2.addr = alloca i32, align 4
 ^
<stdin>:8:7: note: previous match ended here
entry:
      ^
<stdin>:9:1: note: non-matching line after previous match is here
 %param3.addr = alloca float, align 4
^

Input file: <stdin>
Check file: C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<
        .
        .
        .
        5:  
        6: ; Function Attrs: noinline nounwind optnone 
        7: define dso_local void @t1(i32 noundef %param1, i32 noundef %param2, float noundef %param3) #0 { 
        8: entry: 
        9:  %param3.addr = alloca float, align 4 
       10:  %param2.addr = alloca i32, align 4 
next:7      !~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  error: match on wrong line
       11:  %param1.addr = alloca i32, align 4 
       12:  store float %param3, float* %param3.addr, align 4 
       13:  store i32 %param2, i32* %param2.addr, align 4 
       14:  store i32 %param1, i32* %param1.addr, align 4 
       15:  ret void 
        .
        .
        .
>>>>>>

error: command failed with exit status: 1

--

I can't see a scenario where we would need this with a variadic call. Ultimately these wrap specific physical instructions, not some kind of arbitrary call

Excellent, that removes that concern. Thanks! Aside from the test failures and a minor coding style nit, I think this is close to ready to go.

clang/lib/CodeGen/CGCall.cpp
2063–2065
skc7 updated this revision to Diff 448330.Jul 28 2022, 7:11 AM
skc7 marked an inline comment as not done.

Fix tests failures.

aaron.ballman added inline comments.Jul 28 2022, 8:15 AM
clang/test/CodeGen/attr-maybeundef-template.cpp
4

This mangling works for Itanium targets, but doesn't match MSVC targets, which is why the test is failing on Windows.

clang/test/CodeGen/attr-maybeundef.c
49

It looks like dso_local is missing here on Windows and expected to be missing on Linux? Not certain what's up with that...

skc7 updated this revision to Diff 448361.Jul 28 2022, 9:28 AM

Fix windows tests failure

skc7 added inline comments.Jul 28 2022, 9:29 AM
clang/test/CodeGen/attr-maybeundef.c
49

On Linux, dso_local isn't present.
declare void @VariadicFunction(i32 noundef, ...)

This revision is now accepted and ready to land.Jul 28 2022, 12:19 PM
nlopes added a subscriber: nlopes.Jul 29 2022, 1:20 AM

Would it be possible to implement this by dropping the noundef attribute rather than emitting a freeze? Seems like a much better option to me.

Would it be possible to implement this by dropping the noundef attribute rather than emitting a freeze? Seems like a much better option to me.

See @nhaehnle's post here

Would it be possible to implement this by dropping the noundef attribute rather than emitting a freeze? Seems like a much better option to me.

See @nhaehnle's post here

Thank you for the reference!
Seems like a big hammer, but I don't know the semantics of the GPU languages to comment further.

Hi, the test cases that this patch introduces are failing on some ppc64le (Linux on Power) buildbots:
https://lab.llvm.org/buildbot/#/builders/57
https://lab.llvm.org/buildbot/#/builders/230

Would you mind taking a look please?

If we don't hear from @skc7 in the next ~hour with a fix, feel free to revert to get the bots back to green.

If we don't hear from @skc7 in the next ~hour with a fix, feel free to revert to get the bots back to green.

Issue is with missing target triple in the tests. Submitted D130790 for review, which should fix the tests.

amyk added a comment.Jul 29 2022, 11:29 AM

If we don't hear from @skc7 in the next ~hour with a fix, feel free to revert to get the bots back to green.

Thanks @aaron.ballman. I've reverted the patch for the time being: https://reviews.llvm.org/rG4e1fe968c9de73507a1bf0c8aa57e06be457816e

amyk added a comment.Jul 29 2022, 11:30 AM

If we don't hear from @skc7 in the next ~hour with a fix, feel free to revert to get the bots back to green.

Issue is with missing target triple in the tests. Submitted D130790 for review, which should fix the tests.

I realized I didn't happen to see this comment in time and had already reverted the patch. My apologies on this.

skc7 added a comment.Jul 31 2022, 11:07 PM

If we don't hear from @skc7 in the next ~hour with a fix, feel free to revert to get the bots back to green.

Issue is with missing target triple in the tests. Submitted D130790 for review, which should fix the tests.

I realized I didn't happen to see this comment in time and had already reverted the patch. My apologies on this.

Relanded the revert with fixes to tests. commit