What are your thoughts on adding a noprofile function attribute in the FE? @MaskRay suggested filing a bug with gcov (below) to get their take on it.

Use "hasFeature"

Test all nop sizes.

Reduce testcase.

Rework 16-bit nops to use existing loop.

In D97268#2584819, @jyknight wrote:

In D97268#2583366, @void wrote:

That...shouldn't matter since a NOP is a NOP. Regardless, I removed that modification to simplify the patch.

It _does_ matter. To start with it can affect performance (marginally).

But more importantly, if you're doing tricky things with runtime code modification, it's a correctness issue. A trap won't leave the instruction pointer in the middle of a long-nop instruction (which you may be about to replace with another instruction), but it can leave it between two distinct nops.

In D97268#2583270, @jyknight wrote:

In D97268#2582826, @void wrote:

In D97268#2581555, @jyknight wrote:

This change also modifies (breaks!) the 32-bit NOP set, definitely shouldn't do that. (But I think all that can be reverted, in any case).

Which part did it break?

It replaced the single-instruction NOPs with new sequences consisting of multiple instructions.

In D97268#2583270, @jyknight wrote:

In D97268#2582826, @void wrote:

In D97268#2581555, @jyknight wrote:

This change also modifies (breaks!) the 32-bit NOP set, definitely shouldn't do that. (But I think all that can be reverted, in any case).

Which part did it break?

It replaced the single-instruction NOPs with new sequences consisting of multiple instructions.

Correct comments with proper disassembly.

Reduce size and effect of patch.

In D97268#2581555, @jyknight wrote:

This change also modifies (breaks!) the 32-bit NOP set, definitely shouldn't do that. (But I think all that can be reverted, in any case).

Fixed variable name and removed excess test lines.

• Add FileCheck to testcase.

Move testcase.

In D94470#2494271, @MaskRay wrote:

In D94470#2493961, @void wrote:

In D94470#2493295, @MaskRay wrote:

The test should be added in test/Transforms/PGOProfile/, see D87435

The error isn't in opt but in llc though. In fact, I don't think it's specific to PGO, but only triggered by it.

Then it should be in llvm/test/Transforms/LoopStrengthReduce/

In D94470#2494138, @nickdesaulniers wrote:

In D94470#2493986, @void wrote:

In D94470#2493138, @nickdesaulniers wrote:

I guess the presence of the profile data makes it difficult to use bugpoint to reduce the test case more?

It looks like I was able to reduce it. PTAL.

The test case passes for me on ToT; was it over-reduced perhaps?

In D94470#2493138, @nickdesaulniers wrote:

I guess the presence of the profile data makes it difficult to use bugpoint to reduce the test case more?

Reduce the testcase.

In D94470#2493295, @MaskRay wrote:

The test should be added in test/Transforms/PGOProfile/, see D87435

In D92156#2458787, @jsji wrote:

Also, looks like there is bug for tlbie as well, there is NO one operand mnemonic, only 5 or 2.

tlbie serves as both a basic and an extended mne-monic.  The Assembler will recognize a tlbie mne-monic with five operands as the basic form, and a tlbie mnemonic with two operands as the extended form.    In  the  extended  form  the  RIC,  PRS,  and  Roperands are omitted and assumed to be 0.

Ping #2.

Ping.

Are there tests to ensure that the appropriate stack protector level is added to the function after inlining?

Move % eater to MatchRegisterName.

In D91735#2403921, @nickdesaulniers wrote:

I was thinking "would it be better to move this logic down into PPCAsmParser::MatchRegisterName? Well, I would suppose that would depend on if the % prefix on registers is valid in other contexts, for instance maybe other places that directly call PPCAsmParser::MatchRegisterName rather than `PPCAsmParser::tryParseRegister which you've modified here.

In D87956#2348433, @nickdesaulniers wrote:

In phab here, it looks like my newly added clang/test/Frontend/optimization-remark-missed-inline-stack-protectors.c fails on windows because I redirect stdout to /dev/null. How does that work for other tests? I see other tests in that dir write to /dev/null. Oh, I guess -o /dev/null will just create a file called "/dev/null" on Windows? So I should do that rather than 1> /dev/null?

Only a couple of nits, but I think this looks good.

Rerun with the correct llc.

Update comment and testcase.

In D88823#2314595, @jyknight wrote:

Okay. Abandoning this change.

Sorry, maybe I buried the lede too far into the message? "I think the fix you have here is OK" was at the end. :)

One thing, I believe we're trying to turn "asm goto" into something it was never meant to be. If you look at gcc's output for this code.

Okay. I'll reclaim this revision.

One thing, I believe we're trying to turn "asm goto" into something it was never meant to be. If you look at gcc's output for this code:

Okay. Abandoning this change.

Here are the relevant parts of the original issue in get_partial_node. Blocks bb.16.bb100 and bb.17.bb106 are the sources of the values in the PHI node in bb.18.bb110. The bb.19.bb17.i.i.i block is the indirect destination and bb.20.bb113 the default destination.

In D88823#2313337, @jyknight wrote:

When we go to resolve this PHI node, there's no place to put the put the values that works in all situations; we can't split the critical edges and it's not valid to place the resolved instructions at the ends of the predecessor blocks.

We can (at least: should be able to) handle a PHI node in the indirect targets of an INLINEASM_BR, for any value which is not the output of the INLINEASM_BR itself. We place the required register copies prior to the INLINEASM_BR, rather than after it. This is handled by findPHICopyInsertPoint -- where we did have a bug already, fixed in f7a53d82c0902147909f28a9295a9d00b4b27d38.

In D88823#2313262, @nickdesaulniers wrote:

%9:gr32 = PHI %0:gr32, %bb.1, %5:gr32, %bb.2

The problem is this PHI here. We don't have a way to resolve PHI nodes in the entry block to an indirect destination. (This is the reason we don't allow "asm goto" outputs on the indirect paths.) When we go to resolve this PHI node, there's no place to put the put the values that works in all situations; we can't split the critical edges and it's not valid to place the resolved instructions at the ends of the predecessor blocks.

We're also not using "asm goto" outputs in this test case, so I still don't understand why this PHI is problematic.

That doesn't matter. It's problematic because of the PHI in the block with the INLINEASM_BR. As I mentioned, the result of the tail duplication is that that PHI is more-or-less moved *down* into the indirect and default destinations. Look at those PHIs and you'll see that they have the same values coming into them.

In D88823#2312933, @nickdesaulniers wrote:

I confirmed that this patch does fix the kernel panic observed in https://github.com/ClangBuiltLinux/linux/issues/1125#issuecomment-703890483.

I don't understand the bug though. Looking at the output of:

llc -mtriple=i686-- -stop-after=early-tailduplication -print-after-all llvm/test/CodeGen/X86/tail-dup-asm-goto.ll -o - 2>&1 | less

Before/After IR Dump After Early Tail Duplication before rebuilding clang with this patch applied (so still in a bad state), but nothing looks wrong to me. So what precisely is the bug?

Before early tail dup:

bb.0.bb:
  successors: %bb.1(0x40000000), %bb.2(0x40000000); %bb.1(50.00%), %bb.2(50.00%)

  %2:gr32 = IMPLICIT_DEF
  %0:gr32 = MOV32rm killed %2:gr32, 1, $noreg, 0, $noreg :: (load 4 from `i8** undef`, align 8)
  %3:gr32_abcd = MOV32r0 implicit-def dead $eflags
  %4:gr8 = COPY %3.sub_8bit:gr32_abcd
  TEST8rr %4:gr8, %4:gr8, implicit-def $eflags
  JCC_1 %bb.2, 5, implicit $eflags
  JMP_1 %bb.1

bb.1.bb100:
; predecessors: %bb.0
  successors: %bb.3(0x80000000); %bb.3(100.00%)

  %6:gr32 = IMPLICIT_DEF
  MOV32mi killed %6:gr32, 1, $noreg, 0, $noreg, 0 :: (store 4 into `i8** undef`, align 8)
  JMP_1 %bb.3

bb.2.bb106:
; predecessors: %bb.0
  successors: %bb.3(0x80000000); %bb.3(100.00%)

  ADJCALLSTACKDOWN32 0, 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp
  CALLpcrel32 @foo, <regmask $bh $bl $bp $bph $bpl $bx $di $dih $dil $ebp $ebx $edi $esi $hbp $hbx $hdi $hsi $si $sih $sil>, implicit $esp, implicit $ssp, implicit-def $esp, implicit-def 
$ssp
  ADJCALLSTACKUP32 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp
  %5:gr32 = MOV32r0 implicit-def dead $eflags

bb.3.bb110:
; predecessors: %bb.2, %bb.1
  successors: %bb.5(0x80000000), %bb.4(0x00000000); %bb.5(100.00%), %bb.4(0.00%)

  %1:gr32 = PHI %5:gr32, %bb.2, %0:gr32, %bb.1

In D88813#2312627, @nickdesaulniers wrote:

test case looks reasonable (I haven't run it pre-patch to see what goes wrong yet). Just curious, is this CL related to D88823?

@jyknight This doesn't have the loop refactored like you asked for. I'll work on that today.

Update with missing elements so that it works.

In D88813#2312195, @nickdesaulniers wrote:

test?

Add testcase.

Add testcase.

Improve comment on why we're limiting tail duplication on INLINEASM_BR instruction.

In D88823#2312092, @jyknight wrote:

Hm, it doesn't seem like it should be a problem to duplicate an inlineasm_br instruction. I'm interested to see what's going wrong here.

In D88438#2304953, @nickdesaulniers wrote:

In D88438#2304941, @void wrote:

I believe that a critical edge may be split on the default path, but not the indirect path.

I don't think https://github.com/llvm/llvm-project/commit/2fe457690da0fc38bc7f9f1d0aee2ba6a6a16ada made a similar distinction?

Yeah. I made an error there...

In D88438#2305068, @efriedma wrote:

The concept of splitting a critical edge is still relatively new to me; any thoughts on *why* it would not be ok to split a critical branch of an indirect-like jump?

The problem has to do with blockaddress.

For normal branches, splitting an edge is usually straightforward: you just introduce a new block that branches to the original block, and then you rewrite the branch in the predecessor. But suppose you have a block with two indirectbr predecessors, and you want to split the edge. In that case, the "rewrite the branch" step doesn't work: you can't rewrite the address argument to the indirectbr. (Or at least, you can't without performing some invasive rewrite like indirectbr->switch lowering.)

I believe that a critical edge may be split on the default path, but not the indirect path.

Erm...Reverted...Sorry.

Clarify commit message.

In D88195#2293559, @nickdesaulniers wrote:

In D88195#2293533, @void wrote:

In D88195#2293165, @nickdesaulniers wrote:

I'm super confused between the commit message and initial hunk, that seem to make sense and probably should go in clang-11 if it's not too late, and the additional tests for modules which the commit message does not address. Were these meant to be separate commits, because otherwise it looks like you uploaded unrelated stuff. Are C++20 modules broken with asm goto, or are you just adding additional test coverage for that?

The assert only triggers for modules, so yeah modules are broken with "asm goto", but only if asserts are enabled.

The assert was removed from AST/Stmt.cpp; it doesn't look module related. Wouldn't it be triggered by ANY GCCAsmStmt? (I have patches that use ASM goto w/ outputs on the kernel, let me try an assertions enabled build). It's not obvious to me why the method modified would only trigger for modules.

Yes, but that particular function is only called during serialization reading. It would trigger for any serialization, this just happens to be for modules. I'll edit the commit message to be clearer.

In D88195#2293165, @nickdesaulniers wrote:

I'm super confused between the commit message and initial hunk, that seem to make sense and probably should go in clang-11 if it's not too late, and the additional tests for modules which the commit message does not address. Were these meant to be separate commits, because otherwise it looks like you uploaded unrelated stuff. Are C++20 modules broken with asm goto, or are you just adding additional test coverage for that?

Fix test.