Thanks!

Turns out there wasn't any conflict, but I've tried to clean this up a little and add a few more tests.

Sounds good. I'm happy with this, if no one else has any issues.

This does seem, from the tests I ran, to reduce codesize on average. Especially on Thumb1.

Is this because we can just use a MOVS and wont have to fill in any higher bits? And MOVS's aren't trivially rematerialisable? And Thumb2/Arm are handled by getT2SOImmVal?

Minor edit to test (and renamed it to tailcall-dup.ll).

Added a tailcall duplication opt test to Transforms/CodeGenPrepare.

Thanks for pointing to D52070. I think I saw that (it gave me the idea for this), but hadn't realised it had come back out.

Sorry for the delay. Sure, I can do that.

OK. Thanks guys.

I seem to have stepped into a bit of a rabbit hole here.. and a lot of things seem to be a bit broken.

It seems a bit odd to not inline an 8byte memcpy on a 64bit system, even at minsize. Perhaps this should be based on whether i64 is a legal type?

The benchmarks came back as about a 0.2% difference in cycle count, and (crucially) there's no way when deciding function alignment to check for OptSize so we'd inevitably pessimize some cases.

That sounds like a good idea. Thanks for the help on this one.

Yep, LGTM!

Now using getABITypeAlignment, plus added a simple test

Interesting question. There are a couple of reasons to think the tradeoffs are different there. First, I'd probably (based purely on intuition rather than data) expect a loop to be executed more times than most functions. Second, when r7 is the FP, a function is almost guaranteed to start with a 16-bit instruction (push {..., r7, lr}).

Yeah, IVDescriptors sounds like a good name to me. Thanks for makeing this cleaner!

Sounds good to me. WIth some optional nits.

LGTM!

Good stuff. The downstream version of this we have uses Subtarget->isMClass() && Subtarget->hasThumb2(). (Dont ask me why it's downstream, I'm not sure. It's from before my time). I think you are right about the M7 though, the results there are just different, not necessarily better.

Constant Island pass has a bit of a reputation for being the source of difficult bugs. With enough Mir testing, perhaps that doesn't need to be the case though. The general idea of turning the adr's into ldr's seems like a good one, maybe it just needs some better machinary in the pass to handle the pool entries more easily?

This looks fine to me, if like you say the over aligning to 16 thing is not something that should come up a lot.

Hello. Good to see this getting some love.

These new changes look good to me.

Thanks for taking a look at this, it would be good to make use of it. You may want to check the llvm-commits mails for more of Daniels thoughts, as they unfortunately didn't make it into phabricator.

LGTM. Thanks for the patch

These classes to me look large enough to warrant their own file. Maybe something like RecurrenceDescriptor.h/cpp? Or a name that captures InductionDescriptor too? I'm not sure what that would be though..

Because I wanted to make sure it worked, I wrote this. Feel free to clean it up and use it as a unittest if you like:

I'm happy with this as is, but if you want the API to be tested and ensure it keeps working, you could add a unittest. Something like unittests/Transforms/Utils/BasicBlockUtilsTest.cpp or the ones is CloningTest.cpp.

I've become less sure about what the various alignments should be here. There seem to be multiple ways to calculate such things, I'm not sure which are best. For example, for want of a better option I've used getPointerAlign in ItaniumRTTIBuilder::BuildTypeInfo.

Moved to CreateGlobalString

We do seem to return false in every other case, I'm not very sure why this would be different and considered a precondition we would assert on.

Yeah, I agree. Looks like it really helps out thumb1 code, and the code in CGP looks generic enough, not aarch64 specific.

Hello. Please upload with full context to make patched easier to read. See https://llvm.org/docs/Contributing.html

Ping. Everyone happy with this now the main part has gone in?

Thanks.

Thanks for this.

It may be better to make this match the alignment on the global directly, not looking through the ADR. I'm not sure how that's done yet.

Many tablegen crashes later.. :)

Hello. What version of llvm is this based on? It's still using DEBUG macros. It will need rebasing onto trunk.

In D51030#1209332, @peter.smith wrote:

I think that this is likely to be the only time in the test suite that there is a R_AARCH64_LD_PREL_LO19 to a data symbol in a shared library. In theory this should generate a R_AARCH64_COPY dynamic relocation. I think that this was fixed as part of the changes made in https://sourceware.org/bugzilla/show_bug.cgi?id=21532 "[AArch64] Allow COPY relocation elimination" as prior to that R_AARCH64_LD_PREL_LO19 wasn't in the switch statement to generate copy relocations. The change made it into binutils 2.29.

Thanks

In D51030#1207378, @peter.smith wrote:

• I can't see anything obviously wrong with the BFD code for handling LO19

Thanks Peter. This change just adds a FIXME for tiny+tls.

Unfortunately I'm hitting a number of linker problems in testing this.

• Armlink seems to do fine, but I am testing embedded code there.
• ld.bfd links the test-suite fine with fpic (except for the expected errors where relocations are out of range). But without pic gives an error about relocating against stderr that I'm not sure of yet, but might point to this alignment check being wrong in some way. Unfortunately the errors are not clear what is out of range, it may be a legitimate error (although ld.gold and lld both are "successful" at linking the same program).
• ld.gold doesn't like load literal relocations, producing invalid instructions.
• lld, with some extra patches, does fine for anything that isn't a tls relocation I think.

In D49673#1199590, @dmgreen wrote:

Hmm.. So I was expecting this would keep working the same as small model. I believe that would be less efficient but functionally correct (providing you are not un-aligning page boundaries). In adding some tests I can see that there are some code differences though (because it goes through LOADgot). I think the example in arm64-tls-execs is correct, and the other tests here produce the same code (as does your example from above). I've not tested this very well though, and I'm not sure what else would be required. As far as I can tell it should be OK, but producing an error for tiny+tls, same as large+tls, is an option.

In D49673#1198678, @peter.smith wrote:

Generally looking good to me. Apologies for the delay.

Thanks for the review

I'm no expert, but I thought that SmallPtrSet->erase(BB) would treat BB as an opaque value that it removes from it's set, not accessing it's content in any way. You may be right about the eraseFromParent invalidating things though.

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

I was testing the code and ran into some problems with debug metadata on the loop nodes (actually using -Rpass=unroll in that case). Can you make sure that works as expected?

Ping

In D50511#1196431, @dmgreen wrote:

The pass will also need "new pass manager" code at some point.

This looks like a sensible idea.