Page MenuHomePhabricator

reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 23 2013, 6:32 PM (422 w, 3 d)

Recent Activity

Wed, Nov 24

reames added a comment to D114453: [unroll] Split full exact and full bound unroll costing [NFC].

ping

Wed, Nov 24, 2:31 PM · Restricted Project
reames abandoned D114185: [SCEV] Leverage inferred no-self-wrap flags to refine trip counts.

Will reopen when I get time to return to this. Closing in the meantime to get off review queues.

Wed, Nov 24, 2:31 PM · Restricted Project
reames abandoned D114176: [SCEV] Look through invertible functions when infering no-self-wrap from mustprogres.

Will reopen when I get time to return to this. Closing in the meantime to get off review queues.

Wed, Nov 24, 2:30 PM · Restricted Project
reames updated the diff for D113939: [runtime-unroll] Remove restriction about unrolling multiple exit loops.

Address a couple of minor review comments.

Wed, Nov 24, 11:33 AM · Restricted Project
reames requested changes to D113289: LICM: Hoist LOAD without STORE.

Heading in the right direction.

Wed, Nov 24, 11:32 AM · Restricted Project
reames added a comment to D112895: [CVP] Canonicalize signed relational comparisons of scalar integers to unsigned comparison predicates.

Do you mind if I put this change under an on by default cl::opt flag? This way we can temporarily turn it off downstream.

Seems reasonable to me.

Wed, Nov 24, 11:16 AM · Restricted Project

Tue, Nov 23

reames added a comment to D112895: [CVP] Canonicalize signed relational comparisons of scalar integers to unsigned comparison predicates.

Any suggestions for the best place to handle it?

Urgh, this is an annoying case. We need a mixture of dominating facts relating two variables, and range facts about those same variables. We don't really have any single place which has both.

Tue, Nov 23, 4:29 PM · Restricted Project
reames added a comment to D113939: [runtime-unroll] Remove restriction about unrolling multiple exit loops.

Ran the test suite. Surprisingly, this appears to be nearly performance neutral.

Tue, Nov 23, 4:20 PM · Restricted Project
reames committed rG03d8bc184a31: [indvars] Fix lftr crash when preheader is terminated by switch (authored by reames).
[indvars] Fix lftr crash when preheader is terminated by switch
Tue, Nov 23, 9:59 AM
reames updated the diff for D114453: [unroll] Split full exact and full bound unroll costing [NFC].

Further simplify - despite structure of existing code, shouldFullUnroll doesn't modify it's UP param.

Tue, Nov 23, 9:47 AM · Restricted Project
reames requested review of D114453: [unroll] Split full exact and full bound unroll costing [NFC].
Tue, Nov 23, 9:41 AM · Restricted Project
reames added a reverting change for rGb00fc198224e: profi - a flow-based profile inference algorithm: Part I (out of 3): rG065f777d2740: Revert "profi - a flow-based profile inference algorithm: Part I (out of 3)".
Tue, Nov 23, 9:20 AM
reames committed rG065f777d2740: Revert "profi - a flow-based profile inference algorithm: Part I (out of 3)" (authored by reames).
Revert "profi - a flow-based profile inference algorithm: Part I (out of 3)"
Tue, Nov 23, 9:20 AM
reames added a reverting change for D109860: profi - a flow-based profile inference algorithm: Part I (out of 3): rG065f777d2740: Revert "profi - a flow-based profile inference algorithm: Part I (out of 3)".
Tue, Nov 23, 9:19 AM · Restricted Project
reames committed rG18086186ab5a: [unroll] Remove two dead variable assignments [nfc] (authored by reames).
[unroll] Remove two dead variable assignments [nfc]
Tue, Nov 23, 9:12 AM
reames committed rG5c77aa2b917c: [unroll] Use early return in shouldFullUnroll [nfc] (authored by reames).
[unroll] Use early return in shouldFullUnroll [nfc]
Tue, Nov 23, 9:02 AM

Mon, Nov 22

reames added a comment to D111806: [LICM] Check the number of divergent paths from loop header to target block.

To keep the conversation coherent I'll talk about the same feedback for D87551 here:

If we were being presented with hard cases to undo with a cost based transform, I might be willing to entertain the notion that LICM should be non-canonical. As it stands, I don't see such examples.

The original change that prompted adding this from @wenlei is a more complicated situation https://reviews.llvm.org/D65060#1596899. Specifically, once the values are hoisted out of the 200 switch arms there's not longer a dominating use block to sink them back into. In addition, the block frequency information at that point no longer indicates that there's an issue from the hoisting. Getting MachineLICM to handle that case is complicated and I suspect fragile.

FYI, I find this very unconvincing. First, the motivating test case appears to have been discussed, but never shared. Second, as discussed, we appeared to have something along the lines of a switch mapping to address offsets, which is then loaded from. We can and do pull such geps down through the phi, and that optimization should probably have triggered on the discussed example. In general, a missing optimization is a *bad* motivation for disabling a canonicalization.

Mon, Nov 22, 4:26 PM · Restricted Project
reames added a comment to D114185: [SCEV] Leverage inferred no-self-wrap flags to refine trip counts.

I'm really uncomfortable with removing things from UniqueSCEVs. If I understood correctly, your motivation here is that getZeroExtendExpr will not try folds if it has already memoized that the fact that this expression doesn't fold. I think we'd be better off adding a parameter to skip that check for this use-site.

Fair enough, that's why I asked. Will restructure.

Mon, Nov 22, 4:22 PM · Restricted Project
reames added inline comments to D114039: [runtime-unroll] Prune early exits when unrolling multiple exit loops.
Mon, Nov 22, 4:16 PM · Restricted Project
reames added a comment to D108848: [LoopDeletion] Separate logic in breakBackedgeIfNotTaken using symboic max trip count [nfc].

Took a quick look at this. No obvious relation, I'd guess some type of pass ordering and/or analysis invalidation. If you can reduce me something which changes behavior with this pass, I'll take another look.

Mon, Nov 22, 4:13 PM · Restricted Project
reames requested changes to D113289: LICM: Hoist LOAD without STORE.
Mon, Nov 22, 3:57 PM · Restricted Project
reames added a comment to D113289: LICM: Hoist LOAD without STORE.

Ok, let me describe back to you what you're trying to do. Give a case like:

loop {
  a = *g;
  if (a) break;
  *g = a + 1
}

Where g is some loop invariant address, you want to produce a form which looks like this:

a0 = *g
loop {
  a1 = phi (a0, a2)
  if (a1) break;
  a2 = a1 + 1
  *g = a2;
}
Mon, Nov 22, 3:56 PM · Restricted Project
reames added a comment to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

@nikic I don't see anything obvious here, and don't consider the reported issue worthy of revert. Given that, I'm going to stop look into this. If you want to file a standalone bug with a reproducer, I can take a further look, but at the moment, the amount of information shared so far is not really easily actionable.

Mon, Nov 22, 3:20 PM · Restricted Project
reames added inline comments to D114325: Add a best practice section on how to configure a fast builder.
Mon, Nov 22, 11:45 AM · Restricted Project
reames added inline comments to D114325: Add a best practice section on how to configure a fast builder.
Mon, Nov 22, 11:07 AM · Restricted Project
reames committed rG1a76a3a7e42d: [docs] Incorprate first round of feedback on D114325 (authored by reames).
[docs] Incorprate first round of feedback on D114325
Mon, Nov 22, 11:03 AM
reames added a comment to D114325: Add a best practice section on how to configure a fast builder.

Generally seems like a good direction to go in, if it's feasible - but it seems like a pretty big step up in resource expectations than the past/current buildbots/workers, so I'm not sure how feasible it is to get those resources.

Er, no. This is very much documenting current status. While we have a bunch of batch builders, we also have a bunch which do build every commit.

Mon, Nov 22, 10:59 AM · Restricted Project

Sun, Nov 21

reames added inline comments to D114325: Add a best practice section on how to configure a fast builder.
Sun, Nov 21, 12:45 PM · Restricted Project
reames updated the diff for D114325: Add a best practice section on how to configure a fast builder.

Note - original patch has landed. Reopening with revisited wording to address suggestions brought up review to avoid fragmenting discussion. This seems like the least confusing option, let me know if you'd rather I do this with a separate review.

Sun, Nov 21, 12:39 PM · Restricted Project
reames added a comment to D114325: Add a best practice section on how to configure a fast builder.

I went ahead and landed this as is, but plan to do a second pass (with separate review/discussion where appropriate) to incorporate @rengolin additional comments.

Sun, Nov 21, 8:03 AM · Restricted Project
reames committed rG73d52ee7859f: Add a best practice section on how to configure a fast builder (authored by reames).
Add a best practice section on how to configure a fast builder
Sun, Nov 21, 8:02 AM
reames closed D114325: Add a best practice section on how to configure a fast builder.
Sun, Nov 21, 8:01 AM · Restricted Project

Sat, Nov 20

reames added a reviewer for D114325: Add a best practice section on how to configure a fast builder: dblaikie.
Sat, Nov 20, 1:05 PM · Restricted Project
reames requested review of D114325: Add a best practice section on how to configure a fast builder.
Sat, Nov 20, 12:59 PM · Restricted Project

Fri, Nov 19

reames added a comment to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

Looking at callgrind profiles for one test case, the main additional cost is when simplifying IVs after unrolling, during the willOverflow check that zext/sext both operands. In getZeroExtendExpr we seem to spend more times in various proveNoWrapByXYZ() methods. It's not obvious to me how this change would cause that, I'd more expect the reverse effect (less need to infer additional flags because we added more here).

All I can think of is that maybe we figure out the trip count for some loop, unroll it, and then spend time analyzing the newly introduced IVs? That's really the only interaction I can find.

Fri, Nov 19, 1:25 PM · Restricted Project
reames added a reverting change for rG1a5666acb281: [SCEV] Defer loop property checks from ea12c2cb as late as possible: rG28000587e1a4: [SCEV] Revert two speculative compile time optimizations which made no….
Fri, Nov 19, 8:46 AM
reames added a reverting change for rG734abbad79db: [SCEV] Defer all work from ea12c2cb as late as possible: rG28000587e1a4: [SCEV] Revert two speculative compile time optimizations which made no….
Fri, Nov 19, 8:46 AM
reames committed rG28000587e1a4: [SCEV] Revert two speculative compile time optimizations which made no… (authored by reames).
[SCEV] Revert two speculative compile time optimizations which made no…
Fri, Nov 19, 8:46 AM
reames added a comment to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

Also no difference. I suspect that we're seeing some kind of second order effect here, not an issue in the code itself.

I agree and am going to revert the two speculative changes as they just complicate the code.

Fri, Nov 19, 8:27 AM · Restricted Project

Thu, Nov 18

reames added a comment to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

Nope, this didn't make any measurable difference.

How about 734abbad7?

Thu, Nov 18, 5:20 PM · Restricted Project
reames committed rG734abbad79db: [SCEV] Defer all work from ea12c2cb as late as possible (authored by reames).
[SCEV] Defer all work from ea12c2cb as late as possible
Thu, Nov 18, 5:20 PM
reames added a comment to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

I don't immediately see what could cause this.

Does 1a5666acb help? That's the only thing I could vaguely see causing this.

Thu, Nov 18, 1:48 PM · Restricted Project
reames committed rG1a5666acb281: [SCEV] Defer loop property checks from ea12c2cb as late as possible (authored by reames).
[SCEV] Defer loop property checks from ea12c2cb as late as possible
Thu, Nov 18, 1:48 PM
reames added a comment to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

Your right, that is unexpected. I don't really see anything in this code likely to be slow, do you see anything obvious? If not, I may need help testing a few variants to see what we see.

Thu, Nov 18, 12:51 PM · Restricted Project
reames requested review of D114185: [SCEV] Leverage inferred no-self-wrap flags to refine trip counts.
Thu, Nov 18, 12:48 PM · Restricted Project
reames requested review of D114176: [SCEV] Look through invertible functions when infering no-self-wrap from mustprogres.
Thu, Nov 18, 10:31 AM · Restricted Project
reames committed rGea12c2cb9c42: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit… (authored by reames).
[SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit…
Thu, Nov 18, 10:11 AM
reames closed D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.
Thu, Nov 18, 10:10 AM · Restricted Project
reames updated the diff for D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

Address review comments

Thu, Nov 18, 9:25 AM · Restricted Project
reames added inline comments to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.
Thu, Nov 18, 9:09 AM · Restricted Project
reames committed rG180625fcb478: Add a breadcrumb comment to make debugging a user error when using . (authored by reames).
Add a breadcrumb comment to make debugging a user error when using .
Thu, Nov 18, 9:07 AM
reames accepted D111133: [AARCH64] Teach AArch64FrameLowering::getFrameIndexReferencePreferSP really prefer SP..

LGTM

Thu, Nov 18, 9:03 AM · Restricted Project
reames committed rG100df68496d1: [SCEV] Add test coverage for invertible functions of IVs (authored by reames).
[SCEV] Add test coverage for invertible functions of IVs
Thu, Nov 18, 9:02 AM
reames updated the diff for D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

This update reworks the patch, and hopefully makes it a lot more obviously correct. The patch is restructured to purely pull code up from howManyLessThans into the caller so that it handles all condition codes. This does reduce the scope slightly, but I've included planned extensions in the review description which should cover all cases.

Thu, Nov 18, 8:39 AM · Restricted Project

Wed, Nov 17

reames committed rG0623f52a46cf: Autogen a test for ease of update (authored by reames).
Autogen a test for ease of update
Wed, Nov 17, 5:24 PM
reames planned changes to D103991: [SCEV] Move mustprogress based no-self-wrap logic so it applies to all exit conditions.

Next action item here is mine. (Rebase needed, tests can be landed, etc..)

Wed, Nov 17, 2:49 PM · Restricted Project
reames abandoned D104066: [SCEV] Use knowledge of stride to prove loops finite for LT exit count computation.
Wed, Nov 17, 2:28 PM · Restricted Project
reames added a comment to D114112: [SCEVAA] Avoid forming malformed pointer diff expressions.

Do we have some documentation that indicates what we expect alias() to return in the case where instructionCouldExistWitthOperands fails?

Not quite sure what you're asking. If we have two pointers, the conservative result should always be MayAlias. Is that what you meant?

I mean, if we do an alias query with a pair of Instruction* that don't have a common scope, are all AA implementations required to always return MayAlias? Call llvm_unreachable? Or can we perform some sort of approximation of the possible values produced by the Instruction? If we do approximate the value, what are the rules for that?

Ah, this is a question about the alias interface on AliasAnalysis. It doesn't appear to address this question. I'm honestly not sure if we could return anything other than MayAlias, and very deliberately tried not to open that can of worms. :)

Wed, Nov 17, 12:43 PM · Restricted Project
reames committed rGad69402f3e19: [SCEVAA] Avoid forming malformed pointer diff expressions (authored by reames).
[SCEVAA] Avoid forming malformed pointer diff expressions
Wed, Nov 17, 12:38 PM
reames closed D114112: [SCEVAA] Avoid forming malformed pointer diff expressions.
Wed, Nov 17, 12:38 PM · Restricted Project
reames added a comment to D114112: [SCEVAA] Avoid forming malformed pointer diff expressions.

Do we have some documentation that indicates what we expect alias() to return in the case where instructionCouldExistWitthOperands fails?

Not quite sure what you're asking. If we have two pointers, the conservative result should always be MayAlias. Is that what you meant?

Wed, Nov 17, 12:12 PM · Restricted Project
reames abandoned D104503: [SCEV] Don't require dominance ordering of add/mul/min/max expressions.

Abandoning this in favor of https://reviews.llvm.org/D114112.

Wed, Nov 17, 11:00 AM · Restricted Project
reames requested review of D114112: [SCEVAA] Avoid forming malformed pointer diff expressions.
Wed, Nov 17, 10:58 AM · Restricted Project
reames abandoned D111353: [SCEV] Extend ability to infer flags to more complicates scopes.

Still no reported issues with the original patch. Abandoning for now, will return if warranted.

Wed, Nov 17, 9:00 AM · Restricted Project

Tue, Nov 16

reames requested review of D114039: [runtime-unroll] Prune early exits when unrolling multiple exit loops.
Tue, Nov 16, 3:54 PM · Restricted Project
reames committed rG8d85e945b20e: [SCEV] Canonicalize X - urem X, Y patterns (authored by reames).
[SCEV] Canonicalize X - urem X, Y patterns
Tue, Nov 16, 11:59 AM
reames closed D114018: [SCEV] Canonicalize X - urem X, Y patterns.
Tue, Nov 16, 11:59 AM · Restricted Project
reames added inline comments to D113939: [runtime-unroll] Remove restriction about unrolling multiple exit loops.
Tue, Nov 16, 11:42 AM · Restricted Project
reames added inline comments to D114018: [SCEV] Canonicalize X - urem X, Y patterns.
Tue, Nov 16, 11:35 AM · Restricted Project
reames requested review of D114018: [SCEV] Canonicalize X - urem X, Y patterns.
Tue, Nov 16, 11:30 AM · Restricted Project
reames requested changes to D113289: LICM: Hoist LOAD without STORE.

This needs much clearer motivation - specifically a clear explanation of what you're trying to achieve and why. Please consider that motivation blocking for further review.

Tue, Nov 16, 11:11 AM · Restricted Project
reames committed rG3dd6d5b62825: [tests] Add coverage for different forms of X - urem X, Y (authored by reames).
[tests] Add coverage for different forms of X - urem X, Y
Tue, Nov 16, 9:27 AM
reames committed rG56ae2cfecf1f: autogen a SCEV test file (authored by reames).
autogen a SCEV test file
Tue, Nov 16, 9:27 AM
reames committed rGed6b69a38f99: Add a hasPoisonGeneratingFlags proxy wrapper to Instruction [NFC] (authored by reames).
Add a hasPoisonGeneratingFlags proxy wrapper to Instruction [NFC]
Tue, Nov 16, 9:00 AM
reames requested changes to D113554: Delete code comments for impossible bugs.

What prevents:
base = some alloc
%iv = {0, +, 1} as i8
%gep = getelementptr i32, i32 *%base, i8 %iv

Tue, Nov 16, 8:53 AM · Restricted Project
reames added a comment to D113835: [CVP] Remove ashr of -1 or 0.

Thanks for the review @reames! I've added another test to cover the edge cases. Can you commit this for me?

Sure. To confirm, this is code you intend to contribute to llvm under the terms of the relevant licenses and agreements, and have the authority to agree to same?

Tue, Nov 16, 8:43 AM · Restricted Project
reames added inline comments to D112734: [SCEVExpander] Drop poison generating flags when reusing instructions.
Tue, Nov 16, 8:41 AM · Restricted Project
reames added a comment to D111133: [AARCH64] Teach AArch64FrameLowering::getFrameIndexReferencePreferSP really prefer SP..

Spent some time educating myself on AArch64 frame lowering. I'm mostly relying on the diagram at the top of AArch64FrameLowering.cpp. Review guidance is about specializing the case where the variable sized regions are empty.

Tue, Nov 16, 8:35 AM · Restricted Project

Mon, Nov 15

reames accepted D111114: [STATEPOINT] Force implicit-def for lpr register..

This is a bit outside my expertise so I'm not thrilled about being the sole reviewer for this, but given no one else has responded and this looks reasonable, I'm approving this to unblock progress.

Mon, Nov 15, 8:17 PM · Restricted Project
reames requested review of D113939: [runtime-unroll] Remove restriction about unrolling multiple exit loops.
Mon, Nov 15, 1:54 PM · Restricted Project
reames committed rG8f95e915cd48: [unroll-runtime] Relax two profitability limitations on multi-exit unrolling (authored by reames).
[unroll-runtime] Relax two profitability limitations on multi-exit unrolling
Mon, Nov 15, 1:00 PM
reames committed rGda327e729078: Fix a misleading FIXME in an unroll test (authored by reames).
Fix a misleading FIXME in an unroll test
Mon, Nov 15, 12:20 PM
reames committed rG423da618354a: [runtime-unroll] Inline canSafelyUnrollMultiExitLoop [NFC] (authored by reames).
[runtime-unroll] Inline canSafelyUnrollMultiExitLoop [NFC]
Mon, Nov 15, 11:40 AM
reames committed rGe99902a8723e: [runtime-unroll] Restructure if-clause to improve readability [NFC] (authored by reames).
[runtime-unroll] Restructure if-clause to improve readability [NFC]
Mon, Nov 15, 11:14 AM
reames accepted D113835: [CVP] Remove ashr of -1 or 0.

LGTM with required changes made.

Mon, Nov 15, 8:52 AM · Restricted Project
reames added a comment to D26149: [DAGCombiner] Match load by bytes idiom and fold it into a single load.

Did you consider doing the load combine at IR level? I'm now trying to combine loads at IR level during the InstCombine process, so I'm wondering why you didn't do that.

There was an IR load combine pass (see D3580 - although never enabled by default AFAIK), but it was removed because it could interfere with other passes like GVN.
https://lists.llvm.org/pipermail/llvm-dev/2016-September/105291.html
https://lists.llvm.org/pipermail/llvm-dev/2019-September/135052.html

Mon, Nov 15, 8:45 AM · Restricted Project

Sun, Nov 14

reames accepted D112573: [IndVarSimplify] Reduce nondeterministic behaviour in visitIVCast..

LGTM. We can revisit more complicate heuristics when we have evidence it matters.

Sun, Nov 14, 2:24 PM · Restricted Project

Fri, Nov 12

reames committed rG37ead201e614: [runtime-unroll] Use incrementing IVs instead of decrementing ones (authored by reames).
[runtime-unroll] Use incrementing IVs instead of decrementing ones
Fri, Nov 12, 4:08 PM
reames added a comment to D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point).

@nikic ping on previous question. It's been a month, and this has been LGTMed. Without response, I plan to land this.

Fri, Nov 12, 3:56 PM · Restricted Project, Restricted Project
reames committed rGde2fed61528a: [unroll] Keep unrolled iterations with initial iteration (authored by reames).
[unroll] Keep unrolled iterations with initial iteration
Fri, Nov 12, 11:41 AM
reames committed rGa1b496be6c71: (re-)Autogen one last unroll-and-jam test (authored by reames).
(re-)Autogen one last unroll-and-jam test
Fri, Nov 12, 11:22 AM
reames committed rGf453e23e67e2: Autogen a bunch of unrolling tests for ease of update (authored by reames).
Autogen a bunch of unrolling tests for ease of update
Fri, Nov 12, 10:35 AM
reames accepted D113577: [SCEV] Support rewriting ZExt expressions with loop guard info..

Thanks for the rebase, much easier to understand.

Fri, Nov 12, 10:34 AM · Restricted Project
reames added a comment to D112573: [IndVarSimplify] Reduce nondeterministic behaviour in visitIVCast..

Surely this causes some test diff in the existing test corpus? It looks like the review is incomplete. (Or I'm simply very surprised.)

Fri, Nov 12, 10:10 AM · Restricted Project
reames committed rG5dd64ef528d2: Refresh an autogen test to reduce spurious diffs (authored by reames).
Refresh an autogen test to reduce spurious diffs
Fri, Nov 12, 9:51 AM
reames committed rGe01c91f242fc: [tests] Add coverage for cases we can prune exits when runtlme unrolling (authored by reames).
[tests] Add coverage for cases we can prune exits when runtlme unrolling
Fri, Nov 12, 9:43 AM
reames added a comment to D113577: [SCEV] Support rewriting ZExt expressions with loop guard info..

This generally looks very reasonable.

Fri, Nov 12, 9:19 AM · Restricted Project
reames accepted D113578: [SCEV] Apply loop guards when computing max BTC for arbitrary steps..

LGTM as well. Amusingly, I'd stumbled into a variant of this as well just yesterday. :)

Fri, Nov 12, 9:11 AM · Restricted Project

Wed, Nov 10

reames added a comment to D110527: [llvm-reduce] Add MIR support..

The IR part can often be eliminated from a MIR based test case. But sure, one could argue that when running llvm-reduce it could be mandatory to add an IR section in the .mir file first (but then I think one needs to also add the function prototype).

I can maybe see this as an argument for allowing override of the triple, but not as an argument for not defaulting to the triple if present. Frankly, I find even the override case unconvincing.

Wed, Nov 10, 10:13 AM · Restricted Project
reames added a comment to D110527: [llvm-reduce] Add MIR support..

I'm looking at using this functionality, and ran into a question on the design. I'm not clear why you exposed an mtriple command line argument on llvm-reduce. It appears you're using this to derive a default data layout, and as an input to parsing the machine function. However, LLVM IR allows specifying both data layout and triple in IR. As a result, we should be able to have the parsing derive the appropriate target directly from the test case. Is there something I'm missing here?

I believe it's only used in the MIR case, not the IR case. This could definitely be made clearer in the cl::opt help.

I don't think it's necessary even for the MIR case. MIR includes the full IR module, and is as a result, is also fully self describing. (If the optional datalayout and triple fields are present.)

Wed, Nov 10, 10:02 AM · Restricted Project