Page MenuHomePhabricator

aemerson (Amara Emerson)
Asian George Costanza

Projects

User does not belong to any projects.

User Details

User Since
Sep 9 2013, 3:45 AM (367 w, 3 d)

The sea was angry that day, my friends - like an old man trying to send back soup in a deli.

Recent Activity

Today

aemerson committed rGade6fa46f94b: [AArch64][GlobalISel] Make <8 x s16> for G_INSERT_VECTOR_ELT legal. (authored by aemerson).
[AArch64][GlobalISel] Make <8 x s16> for G_INSERT_VECTOR_ELT legal.
Fri, Sep 25, 2:01 AM
aemerson committed rGf7b36b35b69a: [AArch64][GlobalISel] Manually select G_DUP with s8/s16 gpr scalar operands. (authored by aemerson).
[AArch64][GlobalISel] Manually select G_DUP with s8/s16 gpr scalar operands.
Fri, Sep 25, 2:01 AM

Yesterday

aemerson accepted D87699: GlobalISel: Use unmerge when copying wide vectors to result registers.

LGTM.

Thu, Sep 24, 11:51 AM · Restricted Project
aemerson accepted D88177: [AArch64][GlobalISel] Infer whether G_PHI is going to be a FPR in regbankselect.
Thu, Sep 24, 11:48 AM · Restricted Project
aemerson accepted D88104: [GlobalISel] Combine (xor (and x, y), y) -> (and (not x), y).

Maybe try to modify the existing G_XOR to be a G_AND instead of creating a new instruction?

Thu, Sep 24, 11:35 AM · Restricted Project
aemerson added inline comments to D88191: [AArch64][GlobalISel] Use custom legalization for G_TRUNC for v8i8 vectors.
Thu, Sep 24, 7:54 AM · Restricted Project

Wed, Sep 23

aemerson abandoned D88122: [GlobalISel] Add widenScalar support for G_CONCAT_VECTORS and use for legalization v2s32 G_ICMPs.

I've decided to use custom legalization for this particular case in D88191, abandoning this change but I might revive it in another form later.

Wed, Sep 23, 4:32 PM · Restricted Project
aemerson requested review of D88191: [AArch64][GlobalISel] Use custom legalization for G_TRUNC for v8i8 vectors.
Wed, Sep 23, 4:31 PM · Restricted Project
aemerson abandoned D88120: [GlobalISel] Add artifact combine for trunc(concat_vectors(a, ...) -> concat_vectors(trunc(a), ...).
Wed, Sep 23, 7:15 AM · Restricted Project

Tue, Sep 22

aemerson added a comment to D88120: [GlobalISel] Add artifact combine for trunc(concat_vectors(a, ...) -> concat_vectors(trunc(a), ...).

On further investigation I'm not sure this is the best way forward.

Tue, Sep 22, 11:53 PM · Restricted Project
aemerson requested review of D88122: [GlobalISel] Add widenScalar support for G_CONCAT_VECTORS and use for legalization v2s32 G_ICMPs.
Tue, Sep 22, 4:28 PM · Restricted Project
aemerson requested review of D88120: [GlobalISel] Add artifact combine for trunc(concat_vectors(a, ...) -> concat_vectors(trunc(a), ...).
Tue, Sep 22, 4:23 PM · Restricted Project
aemerson added inline comments to D88104: [GlobalISel] Combine (xor (and x, y), y) -> (and (not x), y).
Tue, Sep 22, 10:19 AM · Restricted Project

Mon, Sep 21

aemerson committed rGa513fdec90de: [AArch64][GlobalISel] Add a post-legalize combine for lowering vector-immediate… (authored by aemerson).
[AArch64][GlobalISel] Add a post-legalize combine for lowering vector-immediate…
Mon, Sep 21, 4:05 PM
aemerson committed rGe3f5046e4477: [AArch64][GlobalISel] Merge selection of vector-vector G_ASHR/G_LSHR and… (authored by aemerson).
[AArch64][GlobalISel] Merge selection of vector-vector G_ASHR/G_LSHR and…
Mon, Sep 21, 4:05 PM
aemerson committed rG825203daae7f: [AArch64][GlobalISel] Make <4 x s16> G_ASHR and G_LSHR legal. (authored by aemerson).
[AArch64][GlobalISel] Make <4 x s16> G_ASHR and G_LSHR legal.
Mon, Sep 21, 3:45 PM

Fri, Sep 18

aemerson committed rG5a50f8b39f4e: [AArch64][GlobalISel] Add legalization and selection support for <4 x s16>… (authored by aemerson).
[AArch64][GlobalISel] Add legalization and selection support for <4 x s16>…
Fri, Sep 18, 11:39 PM
aemerson committed rGcce24bb38d97: [AArch64][GlobalISel] Add tests for pre-existing selection support for <4 x… (authored by aemerson).
[AArch64][GlobalISel] Add tests for pre-existing selection support for <4 x…
Fri, Sep 18, 5:18 PM
aemerson committed rG269bcc39ca87: [AArch64][GlobalISel] Legalize arithmetic ops for <4 x s16> (authored by aemerson).
[AArch64][GlobalISel] Legalize arithmetic ops for <4 x s16>
Fri, Sep 18, 5:18 PM
aemerson committed rG5d34d7f1a0ca: [GlobalISel] Add lowering support for G_ABS and use for AArch64. (authored by aemerson).
[GlobalISel] Add lowering support for G_ABS and use for AArch64.
Fri, Sep 18, 4:17 PM
aemerson closed D87952: [GlobalISel] Add lowering support for G_ABS and use for AArch64.
Fri, Sep 18, 4:17 PM · Restricted Project
aemerson requested review of D87952: [GlobalISel] Add lowering support for G_ABS and use for AArch64.
Fri, Sep 18, 3:15 PM · Restricted Project
aemerson requested review of D87924: [AArch64][GlobalISel] Select all-zero G_BUILD_VECTOR into a zero mov..
Fri, Sep 18, 11:40 AM · Restricted Project
aemerson committed rG615695de27e4: [AArch64][GlobalISel] Make <8 x s8> of G_BUILD_VECTOR legal. (authored by aemerson).
[AArch64][GlobalISel] Make <8 x s8> of G_BUILD_VECTOR legal.
Fri, Sep 18, 10:33 AM

Thu, Sep 17

aemerson committed rGf5898f8c2def: [AArch64][GlobalISel] Make G_STORE <8 x s8> legal. (authored by aemerson).
[AArch64][GlobalISel] Make G_STORE <8 x s8> legal.
Thu, Sep 17, 4:50 PM
aemerson committed rG196e2f97b714: [AArch64][GlobalISel] clang-format AArch64LegalizerInfo.cpp. NFC. (authored by aemerson).
[AArch64][GlobalISel] clang-format AArch64LegalizerInfo.cpp. NFC.
Thu, Sep 17, 4:41 PM
aemerson committed rG7d5b10348371: [AArch64][GlobalISel] Widen G_EXTRACT_VECTOR_ELT element types if < 8b. (authored by aemerson).
[AArch64][GlobalISel] Widen G_EXTRACT_VECTOR_ELT element types if < 8b.
Thu, Sep 17, 11:51 AM
aemerson committed rGbea7749d0364: [AArch64][GlobalISel] Make <8 x s16> and <16 x s8> legal for shifts. (authored by aemerson).
[AArch64][GlobalISel] Make <8 x s16> and <16 x s8> legal for shifts.
Thu, Sep 17, 11:50 AM
aemerson committed rG79b21fc18764: [AArch64][GlobalISel] Fix bug in fewVectorElts action while legalizing oversize… (authored by aemerson).
[AArch64][GlobalISel] Fix bug in fewVectorElts action while legalizing oversize…
Thu, Sep 17, 8:56 AM
aemerson closed D87814: [AArch64][GlobalISel] Fix bug in fewVectorElts action while legalizing oversize G_FPTRUNC vectors.
Thu, Sep 17, 8:56 AM · Restricted Project
aemerson updated the diff for D87814: [AArch64][GlobalISel] Fix bug in fewVectorElts action while legalizing oversize G_FPTRUNC vectors.
Thu, Sep 17, 8:53 AM · Restricted Project

Wed, Sep 16

aemerson requested review of D87814: [AArch64][GlobalISel] Fix bug in fewVectorElts action while legalizing oversize G_FPTRUNC vectors.
Wed, Sep 16, 11:29 PM · Restricted Project
aemerson committed rG6ad33d836033: [AArch64][GlobalISel] Make G_BUILD_VECTOR os <16 x s8> legal. (authored by aemerson).
[AArch64][GlobalISel] Make G_BUILD_VECTOR os <16 x s8> legal.
Wed, Sep 16, 11:20 AM
aemerson accepted D86464: GlobalISel: Lift store value widening restriction.
Wed, Sep 16, 10:43 AM · Restricted Project
aemerson accepted D87530: [AArch64][GlobalISel] Support shifted register form in emitTST.

Add a test for ANDSWrs?

Wed, Sep 16, 10:02 AM · Restricted Project

Tue, Sep 15

aemerson added inline comments to D87654: [AArch64][GlobalISel] Partially port tryShiftAmountMod from AArch64ISelDAGToDAG.
Tue, Sep 15, 1:58 PM · Restricted Project
aemerson added inline comments to D87628: [AArch64][GlobalISel] Select unscaled loads/stores in manual selector.
Tue, Sep 15, 1:56 PM · Restricted Project

Mon, Sep 14

aemerson accepted D87529: [AArch64][GlobalISel] Refactor + improve CMN, ADDS, and ADD emit functions.
Mon, Sep 14, 2:50 PM · Restricted Project

Fri, Sep 11

aemerson added inline comments to D87529: [AArch64][GlobalISel] Refactor + improve CMN, ADDS, and ADD emit functions.
Fri, Sep 11, 11:54 AM · Restricted Project
aemerson added inline comments to D87529: [AArch64][GlobalISel] Refactor + improve CMN, ADDS, and ADD emit functions.
Fri, Sep 11, 11:52 AM · Restricted Project

Thu, Sep 10

aemerson committed rG0448d11a06b4: [AArch64][GlobalISel] Don't emit a branch for a fallthrough G_BR at -O0. (authored by aemerson).
[AArch64][GlobalISel] Don't emit a branch for a fallthrough G_BR at -O0.
Thu, Sep 10, 3:02 PM

Wed, Sep 9

aemerson accepted D87397: [AArch64][GlobalISel] Share address mode selection code for memops.

Nice clean up, LGTM.

Wed, Sep 9, 2:54 PM · Restricted Project
aemerson committed rGa9f79707624f: Add REQUIRES: asserts to a test that uses an asserts only flag. (authored by aemerson).
Add REQUIRES: asserts to a test that uses an asserts only flag.
Wed, Sep 9, 2:31 PM
aemerson committed rGe5784ef8f6c6: [GlobalISel] Enable usage of BranchProbabilityInfo in IRTranslator. (authored by aemerson).
[GlobalISel] Enable usage of BranchProbabilityInfo in IRTranslator.
Wed, Sep 9, 2:31 PM
aemerson closed D86824: [GlobalISel] Enable usage of BranchProbabilityInfo in IRTranslator.
Wed, Sep 9, 2:31 PM · Restricted Project
aemerson committed rG467a07128533: [GlobalISel][IRTranslator] Generate better conditional branch lowering. (authored by aemerson).
[GlobalISel][IRTranslator] Generate better conditional branch lowering.
Wed, Sep 9, 1:16 PM
aemerson committed rGcc76da7adab7: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less. (authored by aemerson).
[GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less.
Wed, Sep 9, 1:16 PM
aemerson closed D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..
Wed, Sep 9, 1:16 PM · Restricted Project
aemerson closed D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..
Wed, Sep 9, 1:16 PM · Restricted Project
aemerson added a comment to D87297: [GlobalISel] Add bailout thresholds to CSEMIRBuilder::dominates() and the localizer..

Hi Amara,

dominates(A, B) tries to determine dominance by walking an iterator from the beginning of the block potentially until the end. In extremely large BBs this can result in very long compile times, since this is called often from the artifact combiner.

Instead of disabling the check, I think it would be best to fix the underlying compile time issue. The solution here would be to switch the list of MachineInstr to numbered instructions so that dominance checks become O(1).
I had meant to look at this for quite some time but didn't get a chance to do it.
Something similar has been done on LLVM IR (see https://reviews.llvm.org/D51664). I wish it was done in a way that could have benefited the Machine IR like I suggested in https://reviews.llvm.org/D51664#1389884, but this ship has sailed!

I understand this is a big under taking so I am fine with the threshold approach in the meantime.

I leave the final approval to Aditya and Matt.

Cheers,
-Quentin

Wed, Sep 9, 12:52 PM · Restricted Project

Tue, Sep 8

aemerson added inline comments to D87297: [GlobalISel] Add bailout thresholds to CSEMIRBuilder::dominates() and the localizer..
Tue, Sep 8, 2:32 PM · Restricted Project
aemerson requested review of D87297: [GlobalISel] Add bailout thresholds to CSEMIRBuilder::dominates() and the localizer..
Tue, Sep 8, 10:14 AM · Restricted Project
aemerson added a comment to D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..

Ping

Tue, Sep 8, 9:27 AM · Restricted Project

Sat, Sep 5

aemerson committed rGd0abc7574953: [GlobalISel] Disable the indexed loads combine completely unless forced. NFC. (authored by aemerson).
[GlobalISel] Disable the indexed loads combine completely unless forced. NFC.
Sat, Sep 5, 9:15 PM

Fri, Sep 4

aemerson added a comment to D87157: [GlobalISel] Add a localizer for copies from physregs and use it in AArch64.

I remember you mention that the change to CallLowering approach to this issue resulted in worse perf or code size. That does seem odd to me since we should be just aping SDAG, so even if we had some regressions we shouldn't be worse overall.

Fri, Sep 4, 5:58 PM · Restricted Project
aemerson added a comment to D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..

Ping

Fri, Sep 4, 10:38 AM · Restricted Project
aemerson added a comment to D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..

Ping

Fri, Sep 4, 10:38 AM · Restricted Project

Thu, Sep 3

aemerson added a comment to D86871: [SelectionDAG] Let NSW/NUW flags be cleared by default in call to getNode()..

Patch updated per review.

I'm questioning why we should have that "isDefined" logic in the first place. It seems to me that any node with "undefined" flags can only be used correctly if any setting of the flags would be semantically correct -- but then we may just as well use the all-zero setting of the flags anyway.

I changed te uses in the AMDGPU backend so that it is now obvious that the only reason for having isDefined() is to allow SelectionDAGBuilder to set the flags at a later point than when creating the nodes.

To get rid of it I think this is needed:

  • Refactor SelectionDAGBuilder to call setValue(DAG.getNode()) with the correct SDNodeFlags directly.
  • SelectionDAGBuilder::Visit(const Instruction) call to setFlags() can be removed.
  • the isDefined() and AnyDefined member of SDNodeFlags can then be removed.

I still believe that even with that change most (all?) uses of setFlags are dangerous as they could introduce a flag into a shared node that may not be appropriate for all existing uses. But maybe that can then be a separate discussion elsewhere.

I may be missing something, but as long as the memoization does a true intersection of the flags per this patch, there should not necessarily be anything broken, or?

I think it would be possible to make things clearer:

  • When a new equivalent node takes over flags from a node it is replacing (for instance during widening), it should pass the flags to getNode() directly or if there are many calls to getNode() maybe call a function like transferFlags(OrigNode, NewNode) at the end of the function.
  • When a new node is created and the values of the flags are given by the context, the flags should be passed directly to getNode().
  • When a single flag needs to be set on an existing node, why not do it directly instead of calling getFlags(), set...(), setFlags() (like SelectionDAGISel currently does for setNoFPExcept).
  • ...?

I added a TODO comment referencing this.

I agree removing the isDefined() check within intersectWith() is a good idea, and if we can do it without regression, we should do so. (And that will certainly fix the original problem Jonas was seeing.

Yes, all the tests are passing :-) With fast-fp, there are 2 files changing on SPEC with a total of just 8 less fused ops (without fast fp, there is no change). This is due to SelectionDAGBuilder now clearing the flags of the fmul and fadd nodes so DAGCombiner will not produce the fma. I would hope this is temporarily acceptable, or?

@aemerson: It seems you introduced the isDefined() with d28f0cd4 - are you OK with this change, and what are your comments now?

I don't have any strong feelings about this. I agree with Sanjay that correctness is more important but it would be good to have some tests that show what we're now failing to optimize.

Thu, Sep 3, 10:27 AM · Restricted Project
aemerson committed rG2878ecc90f1f: [StackProtector] Fix crash with vararg due to not checking LocationSize… (authored by aemerson).
[StackProtector] Fix crash with vararg due to not checking LocationSize…
Thu, Sep 3, 12:09 AM
aemerson closed D87074: [StackProtector] Fix crash with vararg due to not checking LocationSize validity..
Thu, Sep 3, 12:09 AM · Restricted Project

Wed, Sep 2

aemerson requested review of D87074: [StackProtector] Fix crash with vararg due to not checking LocationSize validity..
Wed, Sep 2, 11:21 PM · Restricted Project
aemerson accepted D86709: [GlobalISel] Extend not_cmp_fold to work on conditional expressions.

LGTM.

Wed, Sep 2, 1:27 PM · Restricted Project

Tue, Sep 1

aemerson added a comment to D86383: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _).

Thanks!

Usually, tests that are marked # REQUIRES: asserts are done so because they make use of some code that is only available behind NDEBUG. However, in this case, it's outright crashing on certain inputs when not in debug mode. At a first glance, that doesn't seem like a correct fix to me -- after all, the crashing bug is still there -- but I'm not familiar with this code, so I'm probably missing something?

Tue, Sep 1, 4:04 PM · Restricted Project
aemerson added a comment to D86383: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _).

Thanks, it seems the test doesn't work without asserts enabled. I've re-committed it fixed in 520ab710fb6f9829b4e70fda1dcc91ed4f614d0a

Tue, Sep 1, 2:34 PM · Restricted Project
aemerson added a reverting change for rG8693ddc74371: Revert "[GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _)"…: rG520ab710fb6f: Revert "Revert "[GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred)….
Tue, Sep 1, 2:34 PM
aemerson committed rG520ab710fb6f: Revert "Revert "[GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred)… (authored by aemerson).
Revert "Revert "[GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred)…
Tue, Sep 1, 2:34 PM
aemerson added a comment to D84502: [AArch64][GlobalISel] Implement __builtin_return_address for PAC-RET.

Did you commit this?

Tue, Sep 1, 11:09 AM · Restricted Project
aemerson added a comment to D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..

Ping

Tue, Sep 1, 11:08 AM · Restricted Project
aemerson added a comment to D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..

Ping,

Tue, Sep 1, 11:07 AM · Restricted Project
aemerson committed rG5ded4442520d: [AArch64][GlobalISel] Optimize away a Not feeding a brcond by using tbz instead… (authored by aemerson).
[AArch64][GlobalISel] Optimize away a Not feeding a brcond by using tbz instead…
Tue, Sep 1, 11:06 AM
aemerson closed D86413: [AArch64][GlobalISel] Optimize away a Not feeding a brcond by using tbz instead of tbnz..
Tue, Sep 1, 11:06 AM · Restricted Project
aemerson committed rG8ad8f484b63c: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _) (authored by aemerson).
[GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _)
Tue, Sep 1, 10:58 AM
aemerson closed D86383: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _).
Tue, Sep 1, 10:57 AM · Restricted Project

Mon, Aug 31

aemerson added a comment to D86383: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _).

Ping

Mon, Aug 31, 10:51 PM · Restricted Project
aemerson accepted D86787: GlobalISel: Implement computeNumSignBits for G_SEXTLOAD/G_ZEXTLOAD.
Mon, Aug 31, 4:35 PM · Restricted Project
aemerson accepted D86735: GlobalISel: Implement computeKnownBits for G_UNMERGE_VALUES.
Mon, Aug 31, 4:32 PM · Restricted Project
aemerson accepted D86458: GlobalISel: Artifact combine unmerge of unmerge.
Mon, Aug 31, 4:14 PM · Restricted Project
aemerson added inline comments to D86458: GlobalISel: Artifact combine unmerge of unmerge.
Mon, Aug 31, 10:33 AM · Restricted Project
aemerson updated the diff for D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..

Rebase and move getICmpTrueVal() from an earlier patch to this one.

Mon, Aug 31, 10:17 AM · Restricted Project
aemerson updated the diff for D86383: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _).

Simplify and refactor code with FP case.

Mon, Aug 31, 12:38 AM · Restricted Project

Fri, Aug 28

aemerson requested review of D86824: [GlobalISel] Enable usage of BranchProbabilityInfo in IRTranslator.
Fri, Aug 28, 4:30 PM · Restricted Project
aemerson abandoned D86479: [AArch64][GlobalISel] Don't emit a branch for a fallthrough G_BR..

Actually I'm going to abandon this. I think later CodeGen passes can eliminate these and make better decisions about it.

Fri, Aug 28, 11:56 AM · Restricted Project
aemerson accepted D86786: GlobalISel: Combine out redundant sext_inreg.

computeNumSignBits() does not mean that the value was sign extended. It only returns you the number of top-most bits that are known to be the same. As a result, this also ends up matching G_ZEXTLOAD which is a bug I hit in an earlier attempt at this.

It is correct to match zextload though, you just know 1 fewer bit vs. sextload. It doesn't matter what the source is. This is exactly what DAGCombiner does: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11054

Fri, Aug 28, 10:45 AM · Restricted Project
aemerson added a comment to D86786: GlobalISel: Combine out redundant sext_inreg.

computeNumSignBits() does not mean that the value was sign extended. It only returns you the number of top-most bits that are known to be the same. As a result, this also ends up matching G_ZEXTLOAD which is a bug I hit in an earlier attempt at this.

Fri, Aug 28, 10:24 AM · Restricted Project

Thu, Aug 27

aemerson updated the diff for D86383: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _).
Thu, Aug 27, 3:54 PM · Restricted Project
aemerson updated the diff for D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..

Address comments.

Thu, Aug 27, 2:49 PM · Restricted Project
aemerson added inline comments to D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..
Thu, Aug 27, 2:49 PM · Restricted Project
aemerson updated the diff for D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..

Address comments.

Thu, Aug 27, 2:47 PM · Restricted Project
aemerson updated the diff for D86383: [GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _).

Add support for vectors.

Thu, Aug 27, 2:45 PM · Restricted Project
aemerson added a comment to D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..

The IRTranslator chooses to omit the G_BR if it can use a fallthrough. Should it not do that either, and the verifier enforce always having G_BR and G_BRCOND?

Thu, Aug 27, 12:09 PM · Restricted Project
aemerson added inline comments to D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..
Thu, Aug 27, 12:07 PM · Restricted Project
aemerson added a comment to D86709: [GlobalISel] Extend not_cmp_fold to work on conditional expressions.

Why isn't this caught in InstCombine?

Thu, Aug 27, 9:47 AM · Restricted Project
aemerson added a comment to D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..

I was actually just looking at the original globalisel proposal, and switches were intended to be directly translated into a G_SWITCH instruction. Would it be much effort to move this into a machine transformation?

For AMDGPU we're stuck using LowerSwitch and control flow intrinsics, but in the near-ish future we can directly handle control flow here

Thu, Aug 27, 12:44 AM · Restricted Project

Wed, Aug 26

aemerson added inline comments to D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..
Wed, Aug 26, 5:02 PM · Restricted Project
aemerson requested review of D86665: [GlobalISel][IRTranslator] Generate better conditional branch lowering..
Wed, Aug 26, 5:00 PM · Restricted Project
aemerson requested review of D86664: [GlobalISel] Rewrite the elide-br-by-swapping-icmp-ops combine to do less..
Wed, Aug 26, 4:58 PM · Restricted Project
aemerson accepted D85199: GlobalISel: Add generic instructions for memory intrinsics.

LGTM.

Wed, Aug 26, 3:54 PM · Restricted Project
aemerson added a comment to D86583: AMDGPU/GlobalISel: Tolerate negated control flow intrinsic outputs.

Great, thanks for doing this.

Wed, Aug 26, 10:55 AM · Restricted Project

Aug 25 2020

aemerson added a comment to D83468: [Debuginfo] Fix for PR46653.

But it's possible that the right solution is to drop the location, rather than use line zero here. Perhaps this code in GlobalISel should be using updateLocationAfterHoist ( https://reviews.llvm.org/D85670 ) @vsk @aprantl

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2183

// We only emit constants into the entry block from here. To prevent jumpy
 // debug behaviour set the line to 0.
 if (const DebugLoc &DL = Inst.getDebugLoc())
   EntryBuilder->setDebugLoc(
       DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
 else
   EntryBuilder->setDebugLoc(DebugLoc());
void Instruction::updateLocationAfterHoist() {
  const DebugLoc &DL = getDebugLoc();
  if (!DL)
    return;

Code Snippet from link (https://reviews.llvm.org/D85670) is not setting the line number at all.
And as seen/observed ,we can also prevent jumpy behavior by skipping the line number(By not setting to line number zero).

I'm sharing the code snippet to avoid unnecessary revision hindering the actual discussion.Is this Okey ? Or should I revise ?

That snippet is just returning if there was no debug info at all, there's a ! in the if condition.

Aug 25 2020, 10:55 AM · debug-info, Restricted Project
aemerson accepted D86059: GlobalISel: Combine G_ADD of G_PTRTOINT to G_PTR_ADD.
Aug 25 2020, 10:46 AM · Restricted Project