Thanks!

@craig.topper ping, I'd like to get this committed. So would you be happy to continue or for me to commandeer this? Although I would just ask you for the review :)

LGTM, cheers

Hi @craig.topper, so this is interesting because the AArch64 trunc-zext-chain.ll test file is a reproducer for a bug that I couldn't solve. The test function has two versions, because I couldn't figure out if having an undef was the underlying issue. I think it was causing some trouble with GetDemandedBits, but I was unable to figure it out with some very dense logic in the AArch64 backend. So I'm certainly happy to entertain this idea and matching the behaviour of InstCombine/InstSimplify sounds good to me.

Finally got around to benchmarking this and it mainly just results in increased code size.

Abandoning in favour of D118044.

Updated AArch64 tests.

Okay, fair enough. I understand there's a chance of producing some illegal IR with float types for aarch64 so it's probably safer, and easier, to perform the folding here.

You mention unimplemented hooks in D60394 and on the github issue, so why did you go for this approach instead? Solving this in a generic manner seems like the 'good' thing to do, no?

LGTM

As I said previously, the behaviour of the intrinsics should better defined and then this transform should adhere to those semantics. We're already using different pairs of intrinsics for each backend, so this shouldn't be a problem; however, we may have to ensure the Arm backend rejects the 'old' set.loop.iterations just to be safe.

Sorry for missing your last comment @avieira , I've uploaded a suggested change in D115451, though I still think something is off with isSink.

I could alternatively try to avoid generating the trunc

Yes, there's a TODO about avoiding the insertion in TypePromotion::isSink

My knee jerk reaction is that this looks like a lot of code to add little functionality... the pass can already promote phis, so, IIUC, the only functional change is to enable the use of sign extends for their operands? If so, I don't really understand why this couldn't be added more gracefully into the existing search. My handwavey suggestion would be to search from SExt nodes (in the same manner as icmp, but with a search depth limit of 2), create a dedicated function to estimate the cost of promotion and modify the IRPromoter to enable sign extending some sources. Does any of that sound reasonable?

cheers!

Good bit of brain exercise for the morning... Nice addition and cleanup too, thanks. Just a couple of function renaming requests from me.

Thanks.

I would like to start a top-level discussion so everything isn't lost within the review comments, and pull in the Arm people who still work on this... @dmgreen @samtebbs

Doesn't make much sense to me having this on it's own... merging with D91354 seems like the sensible choice.

Okay, fair enough.

cheers!

I didn't think about the extra latency of a uxt cmp, and I wouldn't have thought it useful with GISel either - so this is a pleasant surprise. Do you reckon it's worth sorting out the sign extend dag patterns before enabling? Or do you think that the change is really gonna be in the noise so much that no-one will notice?

Do you think we should stop using default cost kinds entirely?

I think this would be for the best, and I thought I already changed most of the calls to be explicit.

cheers

Ok, will do.

are you happy with this?

Yep!
Thanks @sherwin-dc

Bah! I'm too used to thinking about extend in this case rather than trunc! Please ignore my previous comments!

Unless there's something in the code which prevents BTC=UINT_MAX/TC=0

And I thought there was!

@RKSimon I'm not really doing much work on LLVM currently, so no.... But I'd be happy to help out with reviews if someone else wanted to pick it up.

Cheers!

Thanks.

This made my brain melt a little... anyway, from Section B 2.2.1 of Arm ARM:

Thanks, LGTM

Okay, thanks. Could you just add a couple more tests then, so that we cover steps other than one. Also, if they don't exist already, a couple of tests where the start value is already at the limit.

I think this looks fine to me, and now I'm trying to remember what the difference was which these functions and cannotBe[Min/Max]InLoop. Have you looked into using these?

Thanks

LGTM

Makes sense to me.

Cheers! And no thanks - I've had commit access for a few years.

• Moved PatLeaf to top-level file.
• Added tests for missing FP conditions.
• Added switch tests.

Can you add tests that do explicit xors in the IR to show that it is not folded out where it shouldn't be?

Well this is what I mean, that I don't think this is possible. Writing a test in IR, the br has to take an i1, though this is expanded to i32 during codegen. I could write some IR tests with xors, but they should all be valid. AFAIK, I'd need a way of writing a test input in selection dag form to write a negative test. I have missed some FP conditions in the existing tests, so I'm going to add those.

Thanks all. I've added a PatLeaf to detect a boolean, but I wasn't sure how to write a negative test using LLVM IR considering that the branch always takes an i1. Any ideas?

Hi Yvan,

The X86 test is indeed large! Considering that it's just an unrolled loop with the same sequence repeated MANY times, could you not just delete 3/4 of the loop and update the indvar accordingly?

Maybe? Not something that I'll be continuing with though.

No, don't think I have the time/energy to possibly introduce a bug :)

Nothing else from me.

But why not just order the logic so that it is an NFC?

Shouldn't this be an NFC..? Looks like we should be favouring post-incs for MVE, even at optsize.

Sounds good. How about unifying it to one call to get the type of preferred indexing: none, pre and post?

cheers!

Thanks! Yes, agreed. I've also added a couple of tests before committing, just to check that unrolling is indeed disabled at -Os and -Oz.