In D87418#2273236, @mstorsjo wrote:

Pushed this one now.

Just FTR, keep in mind that if LLD is configured this way, most existing ELF tests will fail, as they don't pass any explicit -m parameter. Not sure if it's worth adding such an option to all of them, or just require building with the option in the default state if one wants to work on LLD and actually run tests.

Weird, I added case test/CodeGen/PowerPC/sink-down-more-instructions-1.mir. Could you do another check?

I'll go ahead and commit this, as it's affecting an user and the fix is simple and obvious. Hopefully post-commit review is acceptable per my understanding of the rules.

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947851, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947724, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947723, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947722, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947689, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947688, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947687, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947684, @arsenm wrote:

Can you revert this? I think that this transform itself is a workaround, and even if it were a good idea, I think it doesn't belong in another loop over the function in finalizeLowering

Could you elaborate on why that would be a workaround? Basically, after instruction selection, the COPY from SGPR to VGPR should be lowered to a native instruction.

Because this should be done after register allocation like is already done. Replacing a copy with something else should only interfere with generic optimizations

That would be too late for MachineCSE and other optimization to remove the redundant COPYs and reduce the register usage. Moving that after RA won't reduce register pressure.

So you are working around something MahcineCSE isn't doing on copies. You could just have MachineCSE do this for these copies

MachineCSE and other optimizations are designed not to handle that target-independent COPY or like, which is added with the intention that, potentially, the source and destination operands are coalesced and that COPY is removed finally. As SGPR and VGPR are different register banks and won't be coalesced anyway, native instruction should be used instead.

This does not make sense, COPY is what generic optimizations do understand. If it's useful to CSE cross bank copies, MachineCSE should handle them. As a representational choice, not-copy is worse than copy

A target-independent cross-bank COPY is definitely useful but the current COPY should not be used for that purpose considering how it's used in RA-related passes, especially only architectural constraints are changed between the source and destination operands where the propagation should be stopped.

In D77491#2282219, @tambre wrote:

In D77491#2282166, @dmajor wrote:

This commit broke Firefox builds on Mac with an error in the SDK headers. Could you please revert if a fix is not readily available?

Reproducer:

struct objc_super {};
extern "C" id objc_msgSendSuper(struct objc_super *super, SEL op, ...);
extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, ...);

Result:

$ clang -c test.mm
test.mm:3:48: error: reference to 'objc_super' is ambiguous
extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, ...);
                                               ^
test.mm:1:8: note: candidate found by name lookup is 'objc_super'
struct objc_super {};
       ^
note: candidate found by name lookup is 'objc_super'
1 error generated.

Looking into this, got an idea what might be wrong. Gimme an hour or two. :)

In D87149#2282224, @spatel wrote:

In D87149#2281145, @uabelho wrote:

Hi!

We've run into a case where instcombine crashes with this patch:

opt -o /dev/null bbi-47401.ll -instcombine

on

@c = external global i16*, align 1

define void @main() {
entry:
  br label %for.cond

for.cond:                                         ; preds = %g.exit, %entry
  %conv1 = phi double [ %conv, %g.exit ], [ undef, %entry ]
  br i1 undef, label %for.end, label %for.body

for.body:                                         ; preds = %for.cond
  %0 = load i16*, i16** @c, align 1
  %1 = load i16, i16* %0, align 1
  %conv = sitofp i16 %1 to double
  unreachable

g.exit:                                           ; No predecessors!
  br label %for.cond

for.end:                                          ; preds = %for.cond
  %conv1.lcssa = phi double [ %conv1, %for.cond ]
  store double %conv1.lcssa, double* undef, align 1
  ret void
}

results in

opt: ../lib/Transforms/InstCombine/InstCombineInternal.h:448: virtual llvm::Instruction *llvm::InstCombinerImpl::eraseInstFromFunction(llvm::Instruction &): Assertion `I.use_empty() && "Cannot erase instruction that is used!"' failed.

I suppose it's the dead block %g.exit that messes up things.

We had a use_empty() guard in an earlier draft of this patch and decided that could not happen. :)
https://reviews.llvm.org/D87149#2259207
Thanks for the test case!

LGTM

Fixed the test case target_parallel_codegen.cpp

In D87149#2281145, @uabelho wrote:

Hi!

We've run into a case where instcombine crashes with this patch:

opt -o /dev/null bbi-47401.ll -instcombine

on

@c = external global i16*, align 1

define void @main() {
entry:
  br label %for.cond

for.cond:                                         ; preds = %g.exit, %entry
  %conv1 = phi double [ %conv, %g.exit ], [ undef, %entry ]
  br i1 undef, label %for.end, label %for.body

for.body:                                         ; preds = %for.cond
  %0 = load i16*, i16** @c, align 1
  %1 = load i16, i16* %0, align 1
  %conv = sitofp i16 %1 to double
  unreachable

g.exit:                                           ; No predecessors!
  br label %for.cond

for.end:                                          ; preds = %for.cond
  %conv1.lcssa = phi double [ %conv1, %for.cond ]
  store double %conv1.lcssa, double* undef, align 1
  ret void
}

results in

opt: ../lib/Transforms/InstCombine/InstCombineInternal.h:448: virtual llvm::Instruction *llvm::InstCombinerImpl::eraseInstFromFunction(llvm::Instruction &): Assertion `I.use_empty() && "Cannot erase instruction that is used!"' failed.

I suppose it's the dead block %g.exit that messes up things.

In D87808#2280197, @dblaikie wrote:

@rsmith What's the deal with these anonymous structs/unions? Why do they have copy/move constructors (are those technically called from the enclosing class's copy/move constructors?) but no default constructor to be called from the other ctors of the enclosing class?

LGTM.

In D77491#2282166, @dmajor wrote:

This commit broke Firefox builds on Mac with an error in the SDK headers. Could you please revert if a fix is not readily available?

Reproducer:

struct objc_super {};
extern "C" id objc_msgSendSuper(struct objc_super *super, SEL op, ...);
extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, ...);

Result:

$ clang -c test.mm
test.mm:3:48: error: reference to 'objc_super' is ambiguous
extern "C" void objc_msgSendSuper_stret(struct objc_super *super, SEL op, ...);
                                               ^
test.mm:1:8: note: candidate found by name lookup is 'objc_super'
struct objc_super {};
       ^
note: candidate found by name lookup is 'objc_super'
1 error generated.

Respond to review comments: change comments, few code revisions, notably with
std::string. Tighten up test cases with more -NEXT checks of the IR.

@ychen, Why are the regexs not sufficient for testing? I would rather not "check the common sub-string of GCC's and MSVC's output" because it has already been sufficiently demonstrated that this is fragile in the test. Referring to the code creating one of the banners:
SmallString<20> Banner = formatv("* IR Dump After {0} *", PassID);
We see that the test checks everything except the PassID (the name is added after) and the rest of the banner is distinct in all cases. I have changed the regexs to {{.+}} which will ensure that there is actually something there but checking anything else will make the test susceptible to failing based on the compiler. The important part of the test is captured in that the banner shows that the dump is either filtered, ignored, output, etc and that the portion of IR is correct. This is the intent of the tests, rather than testing how different compilers construct a PassID.
@MaskRay From earlier comments:
"The reason for the round-trip structure is that there are more instantiations of these templates that report changes in different forms still to come. I will be subsequently posting reviews for at least 2 different change reporters based on these base classes. By defining these templates in this fashion, the functionality of comparing different IR representations and determining when changes have occurred and should be filtered, reported or ignored is all in common in the base classes. The derived classes will be called when interesting events happen and only need to handle presenting the events without having to determine when the events occur."
There are 2 other derived change reporters currently up for review based on these base classes, although the PRs need updating based on changes made in this PR. This makes at least 3 derived classes. See https://reviews.llvm.org/D87000 and https://reviews.llvm.org/D87202

In D87858#2282150, @yaxunl wrote:

In D87858#2280429, @jfb wrote:

Please provide documentation in this patch.

opencl atomic builtins are documented as notes to __c11_atomic builtins part of https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions. These new atomic builtins can be documented in a similar way after that.

Fix typo

rebase

Forgot to approve this patch.

Address clang-format concerns.

I think error is fine because realistically linking modules with different BTI settings will result probably a binary that crash runtime anyway if BTI is on.
otherwise turning BTI on just of a subset of object is useless.

Comment nits.

LGTM; thanks for fixing this.

This commit broke Firefox builds on Mac with an error in the SDK headers. Could you please revert if a fix is not readily available?

In D87859#2280893, @PeteSteinfeld wrote:

In D87859#2280407, @klausler wrote:

NULL() isn't "undefined". It's a pointer (or in some other contexts, an unallocated allocatable), and the description of ASSOCIATED() in 16.9.16 just requires that its first (POINTER=) argument be a pointer.

(Even if NULL() were undefined, it would be okay -- see paragraph (2) of 16.1, which describes general rules for inquiry functions, of which ASSOCIATED is an example.) "Unless the description of an intrinsic inquiry function states otherwise, these arguments are permitted to be unallocated allocatable variables or pointers that are undefined or disassociated."

This all sounds reasonable to me. If what you say is all correct, I would expect the call to ALLOCATED() to return .FALSE.. But every compiler I've tried (IBM, Intel, PGI, and GNU) all produce compile-time errors.

@klausler, is it your guidance that we should compiler the expression ALLOCATED(NULL)) without error?

I strongly dislike build scripts pulling files off the internet. It's bad for machines that aren't connected to the web and upsets people in security

Fixed output_dir cmake variable. Clean build succeeds now.
Ready to land.

LGTM

In D87858#2280429, @jfb wrote:

Please provide documentation in this patch.

I'm aware the ELF supports comdat groups, but when I spoke to @xur back in 2019 we decided it was best not to use this feature. That resulted in D68041, which I see you reverted.

LGTM

Replaced custom selection with isel patterns. Since we're going the isel route I figures I may was well add the missing patterns for unpacked floating point types.

Fix previous reverion of 2004-11-27-SetCCForCastLargerAndConstant.ll and PR17073.ll

LGTM

Rebase on tip and use newly added API;
Slightly change GC pointer operand lowering;

small nits, otherwise LGTM

ELF uses the same comdat group for all the __profd_, __profc_ and __profvp_ symbols can decrease the object file size, but COFF comdat does not support such an operation? @rnk
The build bot link has been garbage collected.

In D87188#2281957, @sanwou01 wrote:

I know this has already been reverted but just FYI that I've bisected a ~2% regression in SPEC2017 x264_r on AArch64 to this commit. Presumably this is due to the extra unrolling / cost modelling issue already mentioned?

Argh, I hit enter too soon. One thing I'd planned to add was I'm sorry about the delay in reviewing this, too.

update LGTM too

Revert 2004-11-27-SetCCForCastLargerAndConstant.ll and PR17073.ll

Removed generated (for review) files.

This is a sensible thing to want and very nice code.
I'm not sure about putting it in JSON.h - there are a few unfortunate aspects that I just don't know how to fix, but limit the benefit which has to be measured against the cost of adding more complexity to the API.

LGTM, though you may want to get a second set of eyes on the swift name validation bits for a more thorough review of that part.

In D87864#2282012, @arsenm wrote:

In D87864#2281901, @rampitec wrote:

I think you need to drop denorm checks and move the check outside of the address space check.

But the DS atomics do respect the denormal mode

In D87864#2281901, @rampitec wrote:

I think you need to drop denorm checks and move the check outside of the address space check.