In D71238#1776518, @reames wrote:

In D71238#1776512, @skan wrote:

In D71238#1776488, @reames wrote:

In D71238#1776443, @skan wrote:

The patch doesn't handle with the hard code case either, but as far as I can see that change was not mentioned in the description of this patch.

Unless I'm misreading the original patch, the notion of "hard code" is only relevant to the prefix padding scheme. Admittedly, I might be misreading, it isn't clearly stated anywhere in the original patch exactly what "hard code" is or what purpose it serves. .

In function X86AsmBackend::alignBranchesBegin, I added the comment

// The prefix or nop isn't inserted if the previous item is hard code, which
// may be used to hardcode an instruction, since there is no clear instruction
// boundary.

I'm sorry if I didn't make it clear. As far as i am concerned, I think the hard code case should be dealt with.

I'm still not following. Can you explain the use case here? What test case differs based on the presence of the "hard code" support in the original patch?

In D71238#1776488, @reames wrote:

In D71238#1776443, @skan wrote:

The patch doesn't handle with the hard code case either, but as far as I can see that change was not mentioned in the description of this patch.

Unless I'm misreading the original patch, the notion of "hard code" is only relevant to the prefix padding scheme. Admittedly, I might be misreading, it isn't clearly stated anywhere in the original patch exactly what "hard code" is or what purpose it serves. .

LGTM

The patch doesn't handle with the hard code case either, but as far as I can see that change was not mentioned in the description of this patch.

In D70157#1775481, @fedor.sergeev wrote:

In D70157#1775016, @annita.zhang wrote:

The point is that we have explicit requirement at the start and we have a lowering into 16-byte sequence that we need to be preserved exactly as it is.
Essentially what we need is a "protection" for this sequence from any changes by machinery that generates the binary code.
How can we protect a particular byte sequence from being changed by this branch aligner?

No, in general we can't. The current solution is based on assembler to insert prefix or nop before the cross (or against) boundary branches. It can only ensure the explicit alignment specified by directive, but not any implicit alignment. I don't think any fixup based on assembler can do it. On the other hand, any code sequence after the alignment directive or even just in a function has some kind of implicit alignment. It's hard for assembler to tell which implicit alignment to preserve. The preferred way is to use explicit alignment directive to specify it.

For your scenario, a NOP padding is more controllable. NOP padding will be inserted just before the branch instructions (or macro fusion branch instructions). So if there's no branches (or macro fusion branches) in your code sequence, there will be no NOP inserted.

What if I insert explicit align(8) right *after* the sequence?

In D70157#1771771, @reames wrote:

We uncovered another functional issue with this patch, or at least, the interaction of this patch and other parts of LLVM. In our support for STATEPOINT, PATCHPOINT, and STACKMAP we use N-byte nop sequences for regions of code which might be patched out. It's important that these regions are exactly N bytes as concurrent patching which doesn't replace an integral number of instructions is ill-defined on X86-64. This patch causes the N-byte nop sequence to sometimes become (N+M) bytes which breaks the patching. I believe that the XRAY support may have a similar issue.

More generally, I'm worried about the legality of arbitrarily prefixing instructions from unknown sources. In the particular example we saw, we had something along the following:

.Ltmp0:

	.p2align	3, 0x90
	(16 byte nop sequence)

.Ltmp3:

	jmp *%rax

In addition to the patching legality issue above, padding the nop sequence does something else interesting in this example. It changes the alignment of Ltmp3. Before, Ltmp3 was always 8 byte aligned, after prefixes are added, it's not. It's not clear to me exactly what the required semantics here are, but we at least had been assuming the alignment of Ltmp3 was guaranteed in this case. (That's actually how we found the patching issue.)

In D70157#1769932, @MaskRay wrote:

I find another deficiency (infinite loop) with the current approach.

Say, there is a je 0 (0x0F 0x84 0x00 0x00 0x00 0x00) at byte 0x90. (0x90+6)%32 == 0, so it ends on a 32-byte boundary.
MF.getMaxPrefixSize() is 4, so the size of MCMachineDependentFragment may vary from 0 to 4.
If there are other MCMachineDependentFragment's in the section, some may shrink while some may expand.
In some cases the following loop will not converge

bool MCAssembler::layoutOnce(MCAsmLayout &Layout) {
  ++stats::RelaxationSteps;

  bool WasRelaxed = false;
  for (iterator it = begin(), ie = end(); it != ie; ++it) {
    MCSection &Sec = *it;
    while (layoutSectionOnce(Layout, Sec)) ///
      WasRelaxed = true;
  }

  return WasRelaxed;
}
 
// In MCAssembler::layoutSectionOnce,
  case MCFragment::FT_MachineDependent:
    RelaxedFrag =
        relaxMachineDependent(Layout, *cast<MCMachineDependentFragment>(I));
    break;

To give a concrete example, clang++ -fsanitize=memory compiler-rt/test/msan/cxa_atexit.cpp -mbranches-within-32B-boundaries does not converge. You may also try dtor-*.cpp in that directory.

A simple iterative algorithm is not guaranteed to converge. We probably can solve the layout problem with a dynamic programming algorithm:

f[i][j] = the minimum cost that layouts the first i instructions with j extra bytes (via NOPs or prefixes)
or
g[i][j] = the minimum inserted bytes that layouts the first i instructions with cost j

I am not clear which one is better. A simple greedy approach is to set an upper limit on the number of iterations when the section contains at least one MCMachineDependentFragment, i.e.

bool HasMCMachineDependentFragment = false;
int count = 5; // arbitrary. Please find an appropriate value.
while (layoutSectionOnce(Layout, Sec, HasMCMachineDependentFragment) && count > 0) {
  WasRelaxed = true;
  if (HasMCMachineDependentFragment)
    count--;
}

In D70999#1769015, @craig.topper wrote:

I wanted this to be the full change from the other patch. Including moving this all to X86BaseInfo.h

Since hardcode only exists in text section, we should not insert HardCodeBegin/HardCodeEnd in other section, such as a virtual section.

We shouldn't try to insert prefix/nop t in a virtual section.

In D70157#1768389, @craig.topper wrote:

Can you please put the macro fusion changes in a separate phabricator review. I’ll review it in the morning US time and if it all looks good we can get that part committed while the other comments are being addressed.

Fix the macro fusion table. If the first source/destination operand of an instruction is memory, it is unfusible.

fix the bug

In D70157#1755927, @jyknight wrote:

An alternative would be to simply emit NOPs before branches as needed. That would be substantially simpler, since it would only require special handling for a branch or a fused-branch. I assume things were done this substantially-more-complex way in order to reduce performance cost of inserting NOP instructions? Are there numbers for how much better it is to use segment prefixes, vs a separate nop instruction? It seems a little bit surprising to me that it would be that important, but I don't know...

I'll note that the method here has the semantic issue of making it effectively impossible to ever evaluate an expression like ".if . - symbol == 24" (assuming we're emitting instructions), since every instruction can now change size. I suspect that will make it impossible to turn this on by default without breaking a lot of assembly code. Previously, only certain instructions, like branches or arithmetic ops with constant arguments of unknown value, could change size.

Speed up if we only want to simply emit NOPs before branches as needed. In this condition, BranchPrefix won't be inserted, which reduces the compile time.

Three changes are made:

1. Remove moveSymbol/updateSymbolMap code since it is not necessary
2. Make variable AlignBoundarySize and variable AlignMaxPrefixSize not global
3. Disable nop padding before instruction with variant symbol operand since it may be rewritten by linker.

In D70157#1755927, @jyknight wrote:

Thanks for the comments, they help a little. But it's still somewhat confusing, so let me write down what seems to be happening:

• Before emitting every instruction, a new MCMachineDependentFragment is now emitted, of one of the multiple types:
  • For most instructions, that'll be BranchPrefix.
  • For things that need branch-alignment, it'll be BranchPadding, unless there's a fused conditional before, in which case it's BranchSplit
  • For fused conditionals, it'll be FusedJccPadding.
• After emitting an instruction that needs branch-alignment, all of those previously-emitted MCMachineDependentFragment are updated to point to the branch's fragment.
• Thus, every MCDataFragment now only contains a single instruction (this property is depended upon for getInstSize, at least).
  
  All the MCMachineDependentFragments in a region bounded by a branch at the end and either a branch or a fragment-type which is not type in {FT_Data, FT_MachineDependent, FT_Relaxable, FT_CompactEncodedInst} at the beginning, will reference the ending branch instruction's fragment.
  
  Then, when it comes time to do relaxation, every one of those machine-dependent-fragments has the opportunity to grow its instruction a little bit. The first instruction in a "block" will grow up to 5 segment prefixes (via modifying the BranchPrefix fragment), and then if more is needed, more prefixes will be added to the next instruction, and so on. Until you run out of instructions in the region. At which point the BranchPadding or FusedJccPadding types (right before the branch/fused-branch) will be able to emit nops to achieve the desired alignment.
  
  An alternative would be to simply emit NOPs before branches as needed. That would be substantially simpler, since it would only require special handling for a branch or a fused-branch. I assume things were done this substantially-more-complex way in order to reduce performance cost of inserting NOP instructions? Are there numbers for how much better it is to use segment prefixes, vs a separate nop instruction? It seems a little bit surprising to me that it would be that important, but I don't know...
  
  I'll note that the method here has the semantic issue of making it effectively impossible to ever evaluate an expression like ".if . - symbol == 24" (assuming we're emitting instructions), since every instruction can now change size. I suspect that will make it impossible to turn this on by default without breaking a lot of assembly code. Previously, only certain instructions, like branches or arithmetic ops with constant arguments of unknown value, could change size.

Not insert nop or prefix if the previous item is hard code, which may be
used to hardcode an instruction, since there is no clear instruction boundary.

In D70157#1747428, @davezarzycki wrote:

In D70157#1746793, @MaskRay wrote:

On x86, the preferred function alignment is 16 (https://github.com/llvm/llvm-project/blob/arcpatch-D70157/llvm/lib/Target/X86/X86ISelLowering.cpp#L1893), which is the default function alignment in text sections. If the cross-boundary decision is made with alignment=32 (--x86-align-branch-boundary=32) in mind, and the section alignment is still 16 (not increased to 32 or higher), the linker may place the section at an address which equals 16 modulo 32, the section contents will thus shift by 16. The instructions that do not cross the boundary in the object files may cross the boundary in the linker output. Have you considered increasing the section alignment to 32?

Shall we default to -mbranches-within-32B-boundaries if the specified -march= or -mtune= may be affected by the erratum?

That isn't good enough. Even though core2 isn't affected by the erratum, core2 code can run on CPUs that do have the bug (and core2 is a popular target for code that needs to run "everywhere"), therefore all target CPUs that predate a hardware fix really

ought to have -mbranches-within-32B-boundaries by default.

In D70157#1746793, @MaskRay wrote:

On x86, the preferred function alignment is 16 (https://github.com/llvm/llvm-project/blob/arcpatch-D70157/llvm/lib/Target/X86/X86ISelLowering.cpp#L1893), which is the default function alignment in text sections. If the cross-boundary decision is made with alignment=32 (--x86-align-branch-boundary=32) in mind, and the section alignment is still 16 (not increased to 32 or higher), the linker may place the section at an address which equals 16 modulo 32, the section contents will thus shift by 16. The instructions that do not cross the boundary in the object files may cross the boundary in the linker output. Have you considered increasing the section alignment to 32?

Shall we default to -mbranches-within-32B-boundaries if the specified -march= or -mtune= may be affected by the erratum?

In D70157#1745128, @craig.topper wrote:

In D70157#1745127, @skan wrote:

In D70157#1745125, @craig.topper wrote:

I've definitely passed something like -Wl,--plugin-opt=-debug-only=isel,dagcombine on the command line before.

I don't know how you could pass it succcessfully. It doesn't work for me either. Here is the complain:

clang --gcc-toolchain=xxxxxx -fuse-ld=gold       -march=skylake-avx512 -mfpmath=sse -Ofast -funroll-loops -flto       -Wl,-plugin-opt=-debug-only=isel,dagcombine         xxx.o main.o             -lm         -o xxx
/usr/bin/ld.gold: error: cannot open dagcombine: No such file or directory

Does it work as -Wl,-plugin-opt="-debug-only=isel,dagcombine" ?

In D70157#1745125, @craig.topper wrote:

I've definitely passed something like -Wl,--plugin-opt=-debug-only=isel,dagcombine on the command line before.

In D62115#1547698, @kimgr wrote:

This looks good to me, thanks!

In D62115#1538631, @kimgr wrote:

I think the test needs a bit more work. It's essentially checking the same thing twice to exercise the Windows path separators.

It looks like there's already a test for -H in FrontEnd/print-header-includes.c. That also demonstrates the use of --check-prefix to handle target-specific stuff. Maybe you could fold this into there?

move test case to print-header-includes.c

change mask paramater' s name from __M to __U in order to be consistent with other intrinsics in the file

make macro more compact

avoid implicit type conversion for intrinsic parameter

uploaded a unrelated patch last time

avoid implicit type conversion for intrinsic parameter

When target feature avx512bf16 is enabled, avx512vl is not enabled implicity for some reasons by now. But _mm_cvtness_sbh, _mm256_cvtpbh_ps , _mm256_maskz_cvtpbh_ps , _mm256_mask_cvtpbh_ps need avx512vl feature. So I move their definition from avx512bf16intrin to avx512vlbf16intrin.h, and move corresponding test cases from avx512bf16-builtins.c to avx512vlbf16-builtins.c.

RegClassOrRegBank is of one of the two classes, but it can still be nullptr, e.g., before regbankselect.
This patch has been reverted in llvm trunk.

ping

make comments for _mm_cvtsbh_ss and _mm_cvtness_sbh better

rm clang-format fix in this patch

make test case consider Windows path separators

In D62115#1511407, @kimgr wrote:

Also, consider ././Inputs/empty.h.

add a test for including headfile outside current directory.

In D62115#1509536, @kimgr wrote:

Should you also consider Windows path separators here?

fix the typo ("Register")

remove ";" and format the code

add "const" to pointer type description.