Rebase.

Address test case comment from dmgreen. Thanks !

In D137108#3899142, @dmgreen wrote:

There are other MOVI nodes created in ISel, have you considered adding support for those too?

MOVI,
MOVIshift,
MOVIedit,
MOVImsl,
FMOV,
MVNIshift,
MVNImsl,

Address comments from reviewers. Thanks !

Address comments from reviewers.

In D129677#3655977, @nickdesaulniers wrote:

In D129677#3653627, @efriedma wrote:

The primary issue with range extension thunks is that they clobber x16 (and in theory are allowed to clobber x17). So we'd need to ensure that all asm goto blocks clobber x16 and x17.

I can't think of any other issue with depending on range extension thunks, I guess. (See also https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#call-and-jump-relocations .)

I guess we could error if the inline asm used x16 or x17 and we did do MFS? IIRC, we already error for 32b r7 being reserved (under some conditions, which I've forgotten). Or maybe avoid MFS if the inline asm had x16 or x17 in its clobber list?

Yes, I think this is a good option. Thanks !

In D129677#3653328, @efriedma wrote:

AArch64 "b" has a range of +-128MB. Which isn't enough for arbitrary programs. So in general, you need a sequence like the following (assuming small code model):

adrp x0, dest
add x0, x0, :lo12:dest
blr x0

That is, unless you're okay with the restriction that your binary is at most 128MB. Which might be reasonable for the Linux kernel, I guess. But again, something you'd want to document...

---

That said, I'm surprised machine function splitting on aarch64 works without any other changes; currently, branch relaxation isn't aware of section markings at all. Or do you have some other out-of-tree patches?

In D129677#3653282, @nickdesaulniers wrote:

In D129677#3653271, @adriantong1024 wrote:

In D129677#3653221, @nickdesaulniers wrote:

In D129677#3653097, @adriantong1024 wrote:

The problem this patch is trying to fix is discovered on AArch64 where the conditional branch b.ge is too short after MFS places hot and cold blocks apart.

Isn't it straightforward to change b.ge to b.lt to the other branch destination, then b (no condition) to the label?

We can do this. I am slightly worried about performance as this is in a performance critical part of the code.

Generally, asm goto is modeled as the indirect branches being taken are the exceptional cases. So the indirect branch targets should be treated as if they were cold. If they're moved far away...good.

Otherwise if that's surprising for that code, it sounds like machine function splitting should be disabled for that function.

In D129677#3653221, @nickdesaulniers wrote:

In D129677#3653097, @adriantong1024 wrote:

The problem this patch is trying to fix is discovered on AArch64 where the conditional branch b.ge is too short after MFS places hot and cold blocks apart.

Isn't it straightforward to change b.ge to b.lt to the other branch destination, then b (no condition) to the label?

In D129677#3649972, @efriedma wrote:

It also seems like such a case is easy to fix; in the inline asm the user should just use the wider encoding.

There's always going to be some limit to the distance unless you use an indirect branch. Well, except on weird targets like x86, where a direct branch can reach everywhere. I guess on targets like AArch64, you could mark the branch destinations as functions, and let the linker could insert a stub. But people writing inline asm probably don't expect a branch to clobber x16/x17...

Once we start imposing any restriction on the distance, we need a patch like this: if the destination is in a different section, we can't promise anything about the distance.

I wouldn't expect this to be an issue on x86, though; even on 64-bit, people normally use the "small" code model, so a "jmp" should reach anywhere in the binary. So I'm not sure what the testcase is supposed to be testing.

In D129677#3649869, @efriedma wrote:

In the inline assembly, there may be branch that can only jump a limited distance, if we run MFS on the function, the resulting distance maybe to far to encode into the instruction.

If there's a rule like this, we should explicitly state in LangRef which branches are/are not allowed.

In D129677#3649806, @efriedma wrote:

The ones in the inline assembly cannot.

This should be fixable on top of D129288. But an incremental fix separate from that would be okay.

Not sure the extra flag carries its weight; it's already hard enough to keep track of all the various flags we put on MachineFunction , and you can already easily check IsInlineAsmBrIndirectTarget() while you're iterating over basic blocks.

In D129677#3649812, @efriedma wrote:

In D129677#3649806, @efriedma wrote:

The ones in the inline assembly cannot.

This should be fixable on top of D129288. But an incremental fix separate from that would be okay.

Err, actually, now I'm confused. What exactly do you mean by "recoded"? Do you mean there's a rule that imposes a maximum distance between an INLINEASM_BR and its indirect destinations? Is that rule written down anywhere?

Update test case.

LGTM

I ran ninja check-lld check-libc check-flang check-llvm check-polly check-bolt check-mlir check-all check-clang check-clang-tools check-openmp locally and things are good.

Address comment from Simon. Thanks!

Rebase after rGadfcdb0d0d4af1194121211c125c291162b43ccd

I am unable to reproduce the failures in check-openmp locally.

Address the comments from @RKSimon. Thanks!
And rebase.

In D25374#3582281, @RKSimon wrote:

In D25374#3582196, @adriantong1024 wrote:

In D25374#565483, @RKSimon wrote:

Updated isNonOpaqueConstantOrConstantVector to isNonOpaqueConstantOrNonOpaqueConstantVector to check that no vector elements are opaque constants. Also added isConstantOrConstantVector helper that doesn't care about opaque. Both of these allow undef vector elements.

Updated the helpers to not allow implicit truncation in build vectors - this will be necessary for the reuse of these helpers in other combines (e.g. SUB and MUL). This isn't necessary for the isNullConstantOrNullSplatConstant case.

Hi @RKSimon

I know this CR is quite old, I am trying to remove the "not allow implicit truncation" restriction for one optimization. I am somewhat new to this part of the compiler, can you please explain to me why this is necessary for SUB and MUL combines. An example would be great. Thanks !

I can't remember tbh - best way to find out would be for you to remove the restriction and run check-llvm-codegen to see what changes/regresses/asserts/explodes....

In D25374#565483, @RKSimon wrote:

Updated isNonOpaqueConstantOrConstantVector to isNonOpaqueConstantOrNonOpaqueConstantVector to check that no vector elements are opaque constants. Also added isConstantOrConstantVector helper that doesn't care about opaque. Both of these allow undef vector elements.

Updated the helpers to not allow implicit truncation in build vectors - this will be necessary for the reuse of these helpers in other combines (e.g. SUB and MUL). This isn't necessary for the isNullConstantOrNullSplatConstant case.

Address comments from Craig. Thanks for the pointer, this does simplify things.

In D127354#3568503, @craig.topper wrote:

This shoudl have been picked up by foldAddSubMasked1 in the generic DAG combiner but wasn't because the BUILD_VECTOR has i8 element type and i32 constant operands. We should fix that to not require the widths to match.

Address comments.

I compiled gcc.c from https://people.csail.mit.edu/smcc/projects/single-file-programs/ and here are the results. This is compiling for ARM64 on an ARM64 machine.

In D125335#3531767, @dmgreen wrote:

I hadn't expected MachineCopyPropgation would be too expensive - it sounds like a simple concept and seems to already be run twice in the default pipeline. This is only adding an extra at -O3, so hopefully won't be too bad for compile time.

Perhaps it is worth running some quick tests though? And if it is a problem adding disablePass(&MachineCopyPropagationID); for AArch64, to run just at the new pass location, not the old. I'm not sure if that would have any knock-on effects, disabling the two existing runs of the pass earlier in the pipeline.

@craig.topper @dmgreen @arsenm Please advise if there is anything else we want to change ?

In D125335#3522676, @arsenm wrote:

Can you avoid adding a second run by moving the current run? The ad-hoc physreg liveness tracking after allocation is really expensive

Address comment from @dmgreen. Thanks for the review.

In D125335#3519829, @lkail wrote:

The problem (my concern) here is some architecture may not be ready to use isCopyInstr to identify what a COPY is.

IIUC, isCopyInstrImpl hasn't been override by many targets and returns None, I think it doesn't affect these targets in MCP. Even when MI.isCopy() returns false, and then isCopyInstr returns true, you still have chance to decide if it's feasible to perform propagation based on this MI's operands' flags and etc. As I have said, you can implement conservative checks in your first patch(sufficient to cover your cases) and enhance it in following patches.

Address comment. Thanks @craig.topper for the time and review.

Address comments.

Address comments

Thanks for the review @dmgreen.

Thanks for the review @craig.topper. I've updated the patch.

Address Craig comment. Thanks for taking a look.

This LGTM

Address Dave's comments. Thanks!

Address Dave's comment. Thanks!

Add 2 more test cases, testing different datatypes.

Address Dave's comment.

Address Dave's comment. Moving the rules into tablegen.

In D114405#3148242, @dmgreen wrote:

Could this be handled with a tablegen pattern? Something like the or_is_add pattern from X86 (but using haveNoCommonBitsSet). Maybe a PatFrag that accepts an add or an add-like-or?

Thanks for the comment David. I will take a look at handling in tablegen pattern as well. Will update when I have more.