- User Since
- Aug 13 2013, 3:10 PM (222 w, 6 d)
Fri, Nov 17
Thu, Nov 16
Wed, Nov 15
I'd be fine with renaming the functions in the benchmark.
Does this work with all linkers? I don't see this document in the manpage of apples ld64.
I'm surprised that lit lets you sneak a dictionary into the reported metrics.
Uh, I wasn't aware that if-conversion could produce terminator instructions in the middle of a basic block, and I would suspect this to break other things like MachineBasicBlock::isReturnBlock() which assume terminators are at the end...
I'd defer to others to comment on the highlevel whether this is a good or helpful idea. Some comments though:
Is this like the existing MachineCopyPropagation pass but working accross multiple blocks? Would D30751 solve your problem as well?
LGTM with nitpicks.
Mon, Nov 13
We should also move TargetCallingConv.h, TargetLowering*.h TargetOpcodes.h, TargetRegisterInfo.h and TargetSubtargetInfo.h. I wonder how we ended up in todays situation...
Mon, Nov 6
Ah yes, should have done this from the start when introducing the subreg syntax.
Wed, Nov 1
Lnt is both a server and a set of script for benchmarking llvm.
Mon, Oct 30
We also have a live installation at http://lnt.llvm.org
A few of our buildbots submit data there.
While the change itself makes sense:
- Please give some more descriptions than "update test" in the future.
- Also give a warning in the commit message as this changes performance and will show up in the LNT numbers.
Thu, Oct 26
Tue, Oct 24
A clear "meh" from my side :)
Feel free to commit it.
Seems obvious, LGTM.
Mon, Oct 23
Oct 17 2017
Without having read the code yet:
I'm not sure I understand what exactly is going on. dead and kill flags work just as well for subregisters; I don't see a single COPY in your MI excerpt so I'm not sure what is going on or how copy propagation could get it wrong.
Oct 16 2017
Note that I took the liberty and changed the commit message to match the new solution.
I must have missed this in the flood of E-Mail. In the future, feel free to ping the review thread when nothing happened for a week.
Oct 13 2017
Oct 12 2017
Turns out this doesn't work nice with the current layering. There are some tools that just link libTarget but not libCodeGen. Merging LLVMTargetMachine and TargetMachine however forces us to move TargetMachine into libCodeGen breaking such users.
Oct 11 2017
Yes that's what I meant.
Just noticed I didn't get around accepting this even though it looks good. LGTM
Oct 10 2017
Some more points on benchmarking (really should rather write more docu than commenting here...):
- Use ninja -j1 to get less noisy compiletime measurements (there of course are a lot of other things you can and should do to reduce the noise level on your system, search for apropriate llvm-dev mailinglist posts etc.)
- On macOS you may need -C ../test-suite/cmake/caches/target-x86_64-macosx.cmake to pick up the right include directories from xcode/xcrun.
- As with any cmake project I find ccmake . really helpful to discover possible options and flags.
- To reduce noise, run the process multiple times and let compare.py take the minimum: compare.py base_run0.json base_run1.json base_run2.json vs patched_run0.json patched_run1.json patched_run2.json (Note: the vs is an actual commandline argument)
For the record, measuring CTMark can be as easy as this without any LNT involved:
You can push this if you want, but I still think that first too loops in fuseInstructionPair() are a poor mans version of the reachability check I outlined in the comments (which would catch more cases).
Machine Copy Propagation does not expect dead stuff to appear so far downstairs. So, it mistakenly replaces live copy with upper one that is marked as dead.
This patch is temporary workaround until Rename Disconnected Subregister Components is fixed.
This is first a bug in machine copy propagation, there is simply no rule that dead copies are not allowed. (If you really want to make it a rule then you really should write a MachineVerifier check for it!)
Do you have any experience how common it is for RenameIndependentSubregs to produce dead code? I'd rather keep that code simpler and purely a renaming operation as the name suggests. Do you know whether the situation is already detected by DetectDeadLanes? I'd be more open to add dead code elimination to that pass (as we could run it instead of dead code elimination I believe).
Thanks for the patch and the testcase, this looks good.
This looks good, but makes me wonder if we shouldn't do the same change (returning a bool instead of looking at the vector containing 0 or more elements) to getUnderlyingObjectsForInstr() for consistency. Could you try doing that?
Please take a look at http://llvm.org/docs/MIRLangRef.html#simplifying-mir-files and simplify the test.
Oct 5 2017
LGTM; maybe wait a bit for @qcolombet to comment.
+1. You could still check the condition in an assert() at the right place though.
This is starting to look good, a few more nitpicks below and hopefullly @iteratee will comment on the isDirective() thing.
Oct 4 2017
Sure feel free to also make such changes in tablegen without review. LGTM
Oct 3 2017
Thanks for working on this!
Thanks @violetav this looks a lot better as a self-contained pass!
Oct 2 2017
Although not necessarily a measure for code quality; here is:
Merging the two isn't too hard either: D38489
And for what it's worth for some of the methods I moved around here:
- usesPhysRegsForPEI() this one should be possible to eliminate with some refactoring where we factor out register scavenging into a separate pass from PEI. (That's somewhere on my TODO list not very high up though).
- useIPRA() this one could just as well be a know in TargetPassConfig.
Yes I mean entire targets.
Tentatively approving this, but please hold on with committing until we have a patch approved that actually uses this.
This seems to be all x86 specific, so I think you should have x86 in the names of CPU_ARCH, DetectCPUArchitecture.c and detect_cpu_architecture.
Feel free to just push without review if you are just changing code to equivalent range-based for constructs like this in the future.
Sep 29 2017
I like this change.
Sep 28 2017
Hmm interesting observation. So to recap (and for further comments in the sourcecode :) you seem to deal with this situation:
- Rebased on ToT.
- Now that we have the Restored member on CalleeSavedInfo we can deal with the ARM/LR problems cleanly.
- Factor out the pristine/reserved register check to a separate commit for now, it's hard enough to get tests passing without them.
- Various fixes.
Sep 27 2017
Sorry about this review stalling. To give at least some feedback:
Thanks for working on it. Looks fine to me, but I think the cmake logic needs adjustment.
Sep 26 2017
That looks cleaner and good to me, thanks.
With phabricator mails back, I just realized that I accidentally committed this yesterday before reviews working, sorry.
Looks like this was committed in r309334
This looks mostly fine.
Looks like this was committed in r312194
Looks like this was committed in r297653
Looks like this was committed in r298162
This landed a while ago in r305625 (as part of D21885)
Looks like this was committed in r310422
Looks like this was committed in r312387
People don't seem to accept static constructors for this.
This was accepted on the mailinglist and committed in r294065