The sea was angry that day, my friends - like an old man trying to send back soup in a deli.
User Details
- User Since
- Sep 9 2013, 3:45 AM (498 w, 5 d)
Fri, Mar 24
@efriedma @spatel @craig.topper what do you think of adding the f8 ValueType here? My SDAG knowledge is rusty, any potential problems?
Mon, Mar 20
Sat, Mar 18
Fri, Mar 3
Also, any tests?
Fix build.
Address comments.
Thu, Mar 2
Feb 28 2023
I’ve thought about adding a MachineValue that acts more like an SDValue which would track the type and index
I’m also wondering for performance reasons whether we should have the types be returned via a single call. With the current patch we call getOperand() twice for each register and I’m not sure if they’ll get optimized to a single call. Maybe:
auto [DstReg, DstTy, Src1Reg, Src1Ty] = MI.getFirst2RegsAndTys();
Feb 27 2023
Thanks, LGTM.
Feb 26 2023
Thanks for the fix. Could you also add a test specific to the bug you're seeing as well? The test case from the GitHub issue is fine.
Feb 24 2023
Use structured bindings with helpers in MachineInstr.
Yes that does seem much cleaner :)
Feb 23 2023
Add some variants that also declare the corresponding types for each register.
Feb 21 2023
Check all defs.
Feb 19 2023
Just realized I wrote the commit message the wrong way around. This bug is due to using > instead of >=.
Feb 18 2023
Feb 17 2023
Feb 15 2023
LGTM. Does this unblock the inliner change?
Feb 10 2023
Feb 9 2023
Address comments. Disable -mandatory-inlining-first by default and insert lifetime intrinsics if not at -O0.
Sure, the exponential compile time case is actually just a side benefit here. The motivating reason for this change is actually to improve code size when building codebases that make heavy use of always_inline.
Feb 8 2023
Feb 7 2023
LGTM, thanks!
Feb 6 2023
LGTM. It would be nice if we could refactor the legalizer to allow better re-use of these rule patterns but that's for another patch.
From my reading of this review, it seems no one has a fundamental disagreement with the overall direction. In that case, I think it's ready to go into a conventional implementation review.
Feb 3 2023
LGTM, could you add a MIR test? We should already have some for the existing build_vector support for other types.
Feb 2 2023
Gentle ping on this discussion. @aaron.ballman
Jan 27 2023
Another ping
Jan 20 2023
Note the size here is not static code size (it excludes cold code). It is actually a proxy to model the runtime cost due to increased instruction footprint (icache pressure). The multiplier's role is to make the savings and 'size' cost comparable in terms of unit. The cycle name here is also counted at IR level, so not at low level.
I understand that, but it still doesn't make the comparison of different units any better. Introducing a scaling factor is irrelevant, it's just an arbitrary scalar.
Right, that's fair enough. See the following comment from the code:
// CycleSavings PSI->getOrCompHotCountThreshold() // -------------- >= ----------------------------------- // Size InlineSavingsMultiplier
Cycles are being compared against code size here. How is that metric supposed to be interpreted? What does (cycles/entry)/(bytes) even mean as a unit of measurement? The feature itself seems to be trying to optimize too fine details in machine cycles in a part of the compiler that shouldn't be dealing with such low level specifics.
FYI I left a comment on https://reviews.llvm.org/D98213 about why I think this feature should not have been enabled.
I want to resurrect this change that enabled these cost-benefit heuristics originally developed in https://reviews.llvm.org/D92780
Jan 17 2023
ping
Jan 13 2023
Jan 11 2023
I haven't had time to properly look at the implementation, but a question on this. When you say "running the test suite", are you talking about building the test suite with -fglobal-isel? Not running the generated code right?
As a separate commit yes that seems fine to me.
Jan 10 2023
Seems perfectly reasonable to me.
Jan 9 2023
LGTM, thanks!
Jan 6 2023
Jan 5 2023
LGTM, thanks!
LGTM with some simplifications.
Dec 7 2022
Dec 5 2022
Nov 23 2022
Remove G_INVOKE_REGION_END
Forgot to remove the END instruction, I’ll do that and update.
Address comments.
Nov 17 2022
Can you link to the bug report? Issue 58432 seems to be unrelated to GlobalISel.
Nov 16 2022
I’m not sure about the inline asm thing since I’m new to this code too, but semantically EH_LABEL isn’t a terminator, it’s a label used for computing exception table information. It seems cleaner to have a dedicated barrier instruction. The reason the legalizations insert at the first terminator is because that guarantees the instruction will execute. If you specifically look for an EH label then it’s leaking implementation details about instruction placement to the API client. Granted, having to use a forward walking ‘getFirstIteratorForward()’ isn’t ideal either.
Nov 15 2022
What do you mean stay as a terminator? Invoke is an IR only instruction that we no longer have after translation.
Nov 13 2022
Nov 12 2022
LGTM, thanks!
Nov 11 2022
Sorry, for some reason I hadn’t received any emails about this so I only noticed now. I’ll look into the issues and come back once the issues are resolved.
Nov 7 2022
Nov 2 2022
Sorry I missed this @, was on parental leave at the time. This is a very nice improvement in code quality, thanks!
Add a clang release note.
Nov 1 2022
Oct 31 2022
Thanks. LGTM.
Oct 28 2022
Thanks for fixing this, could you add a small test as well?