In D77446#1967893, @vsk wrote:

Thanks, lgtm!

Later on, I think we'll want to insert DBG_VALUE insts as well, but this looks very useful as-is.

Add test case

C++ mode comment
Use ore::NV
Fix build errors in legalizer unit test

Add doxygen comment
Remove MF.dump()

In D77575#1965264, @aprantl wrote:

In lieu of a proper pass that strips debug info

What is it that you need? Something like llvm::stripDebugInfo() (from DebugInfo.cpp) but for MIR? That should be very easy to adapt.

In D77575#1964990, @vsk wrote:

Why not generalize this to something like -mir-print-debug-info, which, when set to false, makes the MIR printer stop printing DBG_ meta instructions as well? Then you might not need D77438.

I haven't added a test case for this as D77576 covers it

+ test case

LGTM

LGTM after syncing up with @foad's change to use countLeadingOnes() in DAGISel

LGTM aside from that orphaned comment

LGTM. I expect that it's just that we didn't encounter it or think it could occur

I was going to ask you to use the custom predicate feature but it doesn't appear to be there. I could have sworn I'd committed that before I had to temporarily prioritize other work. It doesn't seem to have been committed and reverted without me noticing either. Oh well, LGTM with the all_combine change and I'll migrate it to custom predicates once I've actually landed that.

LGTM. Sorry for the wait, it shouldn't have taken me this long really as I'd already reviewed the same patch downstream.

I've spoken to Quentin offline and it seems we're not in as much disagreement as it appears. The main issues are:

1. Non-SSA passes aren't included in the plan (I'm focused on IRTranslator to InstructionSelector which are all SSA)
2. Detecting when the contract is violated
3. Not having to write maintenance code

I'll give some more thought to those before I (eventually) make the proposal. I think I have 3. covered, 2. is broadly ok but certainly has room for improvement. For 1. I suggested an off-the-cuff solution that abstracted the cache key to allow implementations to compute a value number by some means instead of directly using the vreg number but it needs some proper thought.

In D74943#1887127, @qcolombet wrote:

That's true (although you'd need a third vreg with no defs/uses to do it) but as you say there's no reason to do so.

That's my point. Even if there is no reason to do so, it is possible.
What I am saying is a mechanism that solely relies on registers not changing value is not bug free.

In D74943#1887089, @arsenm wrote:

In D74943#1887080, @dsanders wrote:

In D74943#1887032, @qcolombet wrote:

My long term plan for the intra/inter-pass cache is based about caching based on the register too.

That seems fragile to do that generally. I agree that is less likely that a register changes, but it is not programmatically impossible. E.g., we could imagine that register numbers get recycle and then we would hit a cache entry that was not intended for a register.

Bottom line, whatever caching we do, it will have to be bound to some kind of observer in my opinion.

We don't recycle vreg numbers today and I'm not aware of any plans to do so. There's also no API to delete a vreg. It's technically possible to change the type but that's potentially dangerous.

At the moment, everyone calls createVirtualRegister() when they need one that didn't exist before and the pre-existing defs at worst become unused by some/all of their uses. A vreg def is never replaced by the def of an instruction that isn't computing (or finishing computing) the same value.

You could swap two unrelated registers with the asme type by uisng replaceRegWith, though I don't know why you would ever do that

In D74943#1887032, @qcolombet wrote:

My long term plan for the intra/inter-pass cache is based about caching based on the register too.

That seems fragile to do that generally. I agree that is less likely that a register changes, but it is not programmatically impossible. E.g., we could imagine that register numbers get recycle and then we would hit a cache entry that was not intended for a register.

Bottom line, whatever caching we do, it will have to be bound to some kind of observer in my opinion.

LGTM

I haven't dug very deep into this but it makes sense to me to be reading the InstInfo and as Simon notes, they should be identical for the cases that already work. On that basis LGTM

Additional comments

LGTM

LGTM

LGTM

LGTM

LGTM

I'm not convinced you really need the FixedLenDecoderEmitter.cpp changes. PPCInstrFormats.td has bits<64> Inst in the I2 class, but PPCDisassembler.cpp has InsnType as being uint32_t . InsnType needs to be able to hold your biggest instruction for the generated code to work correctly. I believe that if you changed:

uint32_t Inst = ...;

in PPCDisassembler::getInstruction(), to:

uint64_t Inst = ...;

then you will no longer need to change FixedLenDecoderEmitter.cpp as you'll be able to use shifts of up to 63.

Fix the remaining small issues

+Testcase

isInteger -> isIntegerLike and comment on pointers needing to use isIntegerLike

I see you've added me as a reviewer but I haven't worked on MIPS for a few years so I'm not really familiar with the current state of their linkers and binaries. AFAIK @atanasyan is still actively working on MIPS so hopefully he can take a look or bring in a colleague who can.
FWIW, this sounds like a good change and the code looks good to me but I think you should get another LGTM from someone more up to date on MIPS.

Thanks everyone. Landed in a5230ac10b0

Add a bit about asking someone to commit for you so that we can document
providing the information needed for the Author property

Small changes for review comments

LGTM

In D69836#1811303, @thakis wrote:

Temporarily reverted in 71d64f72f934631aa2f for now.

In D69152#1808445, @thakis wrote:

This broke tests on Windows: http://45.33.8.238/win/5239/step_11.txt

Probably the "%p doesn't include 0x on Windows" thing again.

In D72309#1807593, @arichardson wrote:

Could we invert the boolean flag to be isFloat? I fear that calling it isInteger, will lead the the same problems that I tried to fix in that commit (calling .isInteger() returns false for pointers).

Corrected patch

LGTM. Thanks!