Rebased and addressed comments from @lattner (I've removed the dead-store-status.ll because it is specific to CleanupPointerRootUsers)

As far as I can tell, the breakage of -fsanitize=leak was not known prior to the commit

What kind of breakage? According to @MaskRay (I've just verified his code) LSAN detects memory leak when valgrind hack is reversed, so there is at least some improvement, not breakage. Even if LSAN needs to keep pointer globals for some reason then it can do so from instrumentation pass, because LSAN is part of LLVM. I see no reason why hacks should reside in LLVM trunk to appease some third party tools.

Just how much do we gain by eliminating these locations? Are we talking 10%, 1%, or what?

I don't know, but preventing optimization to appease valgrind looks .. hm.. strange. I agree with @nlopes that __attribute__((__used__)) should be used for such purpose (probably added from some instrumentation pass)

Fix windows buildbot failure

@nlopes

Not sure you are still interested in this patch. If so, I would suggest you get in touch with some Google folks and check with them if their codebase is ready for this patch. They were the only reason for this workaround.

Regarding the PMUL latency the optimization guide says this. AArch64SchedA57.td probably has older latencies. Should these be updated?

Recommitted in rG993eaf2d69d8

LGTM

In D90844#2433776, @dmgreen wrote:

^ Oh that's a shame. Any idea what was causing the output to be different? This fixed a problem we were seeing (and are now seeing again) in one of our downstream schedules.

Also simplify CodeGenSchedTransition

Patch also lowers time of AArch64 subtarget generation from ~13s to ~10s in Debug build and from ~6s to 2.5s in Release build on my PC

@dmgreen After some studying I came up with different approach: if we get rid of artifical 'AnyCPU' (zero) processor index and explicitly create one PredTransition per processor, we can use much simpler algorithm for variant expansion. Besides fixing read variant compilation issues this also fixes few other things:

@andreadb Built all targets, looks like none is touched (checked RISCV, AMDGPU, PPC, Lanai, etc)

When you say that " Subtarget files are unchanged by the patch", does it mean that you have checked all the targets or just ARM?

Needs to be rebased

Addressed. Revision now depends on D92029

Addressed. Thanks for insights!

Thanks, LGTM

@dmgreen Nice find again, thanks. Somehow I missed that. I've fixed the issue and checked ARM and AArch64 targets - looks good so far.

Addressed

• Added check for Thumb1, because Thumb2 instructions follow different encoding rules
• Added comments

Fixed issue with accessing wrong element in vector

I'm not sure if I see how both halves are related here. Is this a problem that you found, so you altered the A57 to show that issue, so the tablegen code fixes the issue?

Eliminated unused variable

Simplified

@dmgreen Nice find! I've fixed it

Addressed

Ping

Addressed

(paranoia level).
Are we sure that the lowering of MachineInstr to MCInst is preserving the operand sequence? Can it be that the immediate is at position 4 for the MCInst only?
I have no idea how an ldrbt looks like as a MachineInstr. The original check should have triggered an assertion too for MachineInstr then...

Should it always have been checking operand 4 then? I think that makes sense

Uploaded diff with full context

@andreadb Looks like I updated wrong diff last time, sorry. Addressed comments

Rebased and added context

Simplified