Page MenuHomePhabricator

lebedev.ri (Roman Lebedev)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 27 2012, 6:35 AM (388 w, 4 d)

Recent Activity

Today

lebedev.ri accepted D77739: [InstCombine] replace undef in vector constant for safe shift transform (PR45447).

This LG.

Wed, Apr 8, 9:45 AM

Yesterday

lebedev.ri retitled D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables from [clang-tidy] Remove false positive in AvoidNonConstGlobalVariables to [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.
Tue, Apr 7, 12:30 AM · Restricted Project
lebedev.ri added a comment to D77560: [SCEV] CreateNodeForPhi should return SCEVUnknown for incomplete PHIs.

This is missing tests.

Tue, Apr 7, 12:30 AM · Restricted Project

Mon, Apr 6

lebedev.ri accepted D77574: [OpenMP] Fix layering problem with FrontendOpenMP.

LG

Mon, Apr 6, 11:25 AM · Restricted Project
lebedev.ri added inline comments to D77076: [InstSimplify] Allow some arithmetic optimizations for add/sub/div/rem look through freeze.
Mon, Apr 6, 5:54 AM · Restricted Project
lebedev.ri added a comment to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

Mon, Apr 6, 1:36 AM · Restricted Project
lebedev.ri added a comment to D77524: [TargetPassConfig] Add CanonicalizeFreezeInLoops before LSR.

This deserves a PhaseOrdering test (going from the most underoptimized but minimized IR, -O3)

Mon, Apr 6, 12:29 AM · Restricted Project

Sat, Apr 4

lebedev.ri added a comment to D51737: DAG: Combine extract_vector_elt of concat_vectors.

lg

Sat, Apr 4, 11:58 PM
lebedev.ri added a comment to D77454: [WIP] LoadInst should store Align, not MaybeAlign..

I concur, this is in the same general direction as the troubles i was having with these in D76550.

Sat, Apr 4, 10:04 AM · Restricted Project
lebedev.ri added reviewers for D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables: aaron.ballman, lebedev.ri, gribozavr2, JonasToth.
Sat, Apr 4, 8:28 AM · Restricted Project
lebedev.ri added a comment to D75362: [InstCombine] Process blocks in RPO.

This looks good to me.
@spatel?

Sat, Apr 4, 7:24 AM · Restricted Project
lebedev.ri added a comment to D71739: [WIP] Use operand bundles to encode alignment assumptions.

What's the status here?

Sat, Apr 4, 7:24 AM · Restricted Project, Restricted Project
lebedev.ri added a comment to D72529: [SCCIterator] Fix use-after-free.

What's the status here?

Sat, Apr 4, 7:24 AM · Restricted Project
lebedev.ri added a comment to D71692: [WIP] Prototype outlined assumptions and operand bundle information attachment.

With bundle patches landing, what's the status here?

Sat, Apr 4, 7:24 AM · Restricted Project
lebedev.ri added a comment to D66618: [WIP] Expose functions to determine pointer properties (Align & Deref).

What's the status here?

Sat, Apr 4, 7:24 AM · Restricted Project
lebedev.ri requested changes to D77076: [InstSimplify] Allow some arithmetic optimizations for add/sub/div/rem look through freeze.

I don't like pattternmatching changes.
This should just add two new matchers: m_Freeze(<...>) and m_FreezeOrSelf(<...>),
where m_Freeze() matches when the instruction is freeze and the inner matcher matches on the freeze's operand,
and m_FreezeOrSelf(<...>) is m_CombineOr(m_Freeze(<...>), <...>).

Sat, Apr 4, 7:24 AM · Restricted Project
lebedev.ri added a comment to D72550: [SCCIterator] Fix another potential use-after-free.

What's the status here?

Sat, Apr 4, 7:24 AM · Restricted Project
lebedev.ri accepted D77459: [InstCombine] enhance freelyNegateValue() by handling 'not'.

LG
I'll attempt to resurrect Negator patch..

Sat, Apr 4, 7:24 AM · Restricted Project
lebedev.ri accepted D77325: [InstCombine] Don't limit operands in eraseInstFromFunction().

This blames all the way back to rL80483:

inline the trivial AddToWorkList/RemoveFromWorkList methods into their callers. simplify ReplaceInstUsesWith. Make EraseInstFromFunction only add operands to the worklist if there aren't too many of them (this was a scalability win for crazy programs that was only infrequently enforced). Switch more code to using EraseInstFromFunction instead of duplicating it inline. Change some fcmp/icmp optimizations to modify fcmp/icmp in place instead of creating a new one and deleting the old one just to change the predicate.

Sat, Apr 4, 7:24 AM · Restricted Project

Fri, Apr 3

lebedev.ri added a comment to D76572: Replace `T(x)` with `reinterpret_cast<T>(x)` everywhere it means reinterpret_cast. No functional change.

All these changes are rather NFCI, i'm not sure each one needs to be reviewed.

Fri, Apr 3, 2:38 PM · Restricted Project
lebedev.ri added a comment to D77422: [llvm-exegesis] Add benchmark mode that uses LBR for more precise measurements..

The diff is rebase gone wrong.
I personally would think this is going in the wrong direction.
This isn't a new mode, it's still for measuring latency.
I'd think this should add a new X86-specific option "how to measure latency: RDTSC, LBR".

Fri, Apr 3, 12:26 PM · Restricted Project
lebedev.ri retitled D77422: [llvm-exegesis] Add benchmark mode that uses LBR for more precise measurements. from Add benchmark mode that uses LBR for more precise measurements. to [llvm-exegesis] Add benchmark mode that uses LBR for more precise measurements..
Fri, Apr 3, 12:25 PM · Restricted Project
lebedev.ri added a comment to D77417: [BFI][CGP] Add limited support for detecting missed BFI updates and fix one in CodeGenPrepare..

There's already -verify, -verify-dom-info, etc.
I wonder if this should follow similar pattern?

Fri, Apr 3, 12:25 PM · Restricted Project
lebedev.ri added a comment to D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute.

I'm fine with this. I would hope a C/C++/Clang person will also take a look though.

Fri, Apr 3, 11:57 AM · Restricted Project, Restricted Project
lebedev.ri committed rG7d572ef2dd9b: Revert "[SCEV] rewriteLoopExitValues(): even if have hard uses, still rewrite… (authored by lebedev.ri).
Revert "[SCEV] rewriteLoopExitValues(): even if have hard uses, still rewrite…
Fri, Apr 3, 10:16 AM
lebedev.ri added a reverting change for rG44edc6fd2c63: [SCEV] rewriteLoopExitValues(): even if have hard uses, still rewrite if cheap…: rG7d572ef2dd9b: Revert "[SCEV] rewriteLoopExitValues(): even if have hard uses, still rewrite….
Fri, Apr 3, 10:16 AM
lebedev.ri committed rG8e7b25bb401a: [NFC] Move ARM `opt -indvars` test from Codegen into Transforms (authored by lebedev.ri).
[NFC] Move ARM `opt -indvars` test from Codegen into Transforms
Fri, Apr 3, 10:16 AM
lebedev.ri added a comment to rGd0fb6879c37d: [NFC][ARM] Add two tests.

These tests are misplaced, they should be in llvm/test/Transforms/IndVarSimplify/ARM or something, not codegen.
I'll move them.

Fri, Apr 3, 10:15 AM
lebedev.ri added a parent revision for D75908: [SCEV] isHighCostExpansionHelper(): use correct TTI hooks: D73777: [SCEV][IndVars] Always provide insertion point to the SCEVExpander::isHighCostExpansion().
Fri, Apr 3, 9:40 AM · Restricted Project
lebedev.ri added a child revision for D73777: [SCEV][IndVars] Always provide insertion point to the SCEVExpander::isHighCostExpansion(): D75908: [SCEV] isHighCostExpansionHelper(): use correct TTI hooks.
Fri, Apr 3, 9:40 AM · Restricted Project
lebedev.ri accepted D77299: [InstCombine] convert bitcast-shuffle to vector trunc.

LG

Fri, Apr 3, 7:29 AM · Restricted Project
lebedev.ri added a comment to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

After looking more closely at the code I think the issue is within hasLocalStorage() which is called in hasGlobalStorage(). My expectation would be that anything inside of function scope would be considered local but I'm not very certain.
Any thoughts on whether hasLocalStorage() should be modified or if I should change the check and use some more ad-hoc implementation, instead of hasGlobalStorage(), to determine if the variable is local or global?

Fri, Apr 3, 3:11 AM · Restricted Project

Thu, Apr 2

lebedev.ri accepted D76623: [VectorCombine] try to form a better extractelement.

LG for now.

Thu, Apr 2, 3:11 PM · Restricted Project
lebedev.ri added inline comments to D76958: [test-suite] Add support for Hexagon.
Thu, Apr 2, 7:33 AM

Wed, Apr 1

lebedev.ri committed rGde22d7154b4a: [llvm-exegesis] 'Min' repetition mode (authored by lebedev.ri).
[llvm-exegesis] 'Min' repetition mode
Wed, Apr 1, 11:58 PM
lebedev.ri closed D76921: [llvm-exegesis] 'Min' repetition mode.
Wed, Apr 1, 11:57 PM · Restricted Project
lebedev.ri added a comment to D76921: [llvm-exegesis] 'Min' repetition mode.

Great, thank you for the reviews!

Wed, Apr 1, 11:57 PM · Restricted Project
lebedev.ri added inline comments to D76958: [test-suite] Add support for Hexagon.
Wed, Apr 1, 11:23 PM
lebedev.ri added a reviewer for D76983: [InstCombine] Transform extractelement-trunc -> bitcast-extractelement: efriedma.

This patch triggers a regression on our side:

<...>

The tests expects to see:

define dso_local <4 x i16> @truncate_v_v(<4 x i32> %lhs) local_unnamed_addr #0 {
entry:
  %0 = trunc <4 x i32> %lhs to <4 x i16>
  ret <4 x i16> %0
}

which, in machine instructions, is mapped onto a vector trunc instruction.

But now, we see:

define dso_local <4 x i16> @truncate_v_v(<4 x i32> %lhs) local_unnamed_addr #0 {
entry:
  %0 = bitcast <4 x i32> %lhs to <8 x i16>
  %vecinit9 = shufflevector <8 x i16> %0, <8 x i16> undef, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
  ret <4 x i16> %vecinit9
}

which is expanded into a large sequence of code going through the stack.

This looks like a simple missed transform to me, not a miscompile

I agree. We hit a phase ordering difference - SLP can reduce the chain of insert/extract to a vector trunc,
but it doesn't handle the shuffle-of-bitcast. The open question is where to implement that transform.
We're on the edge of instcombine vs. vector-combine if we want to do this in IR.

Wed, Apr 1, 1:09 PM · Restricted Project
lebedev.ri accepted D77230: [InstCombine] enhance freelyNegateValue() by handling xor.

lg

Wed, Apr 1, 10:41 AM · Restricted Project
lebedev.ri added inline comments to D77219: UBSan ␇ runtime.
Wed, Apr 1, 9:00 AM · Restricted Project, Restricted Project
lebedev.ri added a comment to D76974: [Attributor] Make attributor aware of aligned_alloc for heap to stack conversion.

No comments from me.

Wed, Apr 1, 9:00 AM · Restricted Project
lebedev.ri added inline comments to D76958: [test-suite] Add support for Hexagon.
Wed, Apr 1, 8:47 AM
lebedev.ri added a comment to D76921: [llvm-exegesis] 'Min' repetition mode.

@gchatelet thank you for the review!
@courbet any further comments?

Wed, Apr 1, 8:47 AM · Restricted Project
lebedev.ri added a comment to D76983: [InstCombine] Transform extractelement-trunc -> bitcast-extractelement.

This patch triggers a regression on our side:

<...>

The tests expects to see:

define dso_local <4 x i16> @truncate_v_v(<4 x i32> %lhs) local_unnamed_addr #0 {
entry:
  %0 = trunc <4 x i32> %lhs to <4 x i16>
  ret <4 x i16> %0
}

which, in machine instructions, is mapped onto a vector trunc instruction.

But now, we see:

define dso_local <4 x i16> @truncate_v_v(<4 x i32> %lhs) local_unnamed_addr #0 {
entry:
  %0 = bitcast <4 x i32> %lhs to <8 x i16>
  %vecinit9 = shufflevector <8 x i16> %0, <8 x i16> undef, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
  ret <4 x i16> %vecinit9
}

which is expanded into a large sequence of code going through the stack.

Wed, Apr 1, 6:35 AM · Restricted Project
lebedev.ri added a comment to D77211: [LSR] do not replace PHI nodes if its scev is SCEVUnknown.

I feel like the patch doesn't actually describe what the problem is, or why giving up on SCEVUnknown resolves (hides?) it.

Wed, Apr 1, 4:58 AM · Restricted Project
lebedev.ri added inline comments to D76958: [test-suite] Add support for Hexagon.
Wed, Apr 1, 1:03 AM

Tue, Mar 31

lebedev.ri added inline comments to D76974: [Attributor] Make attributor aware of aligned_alloc for heap to stack conversion.
Tue, Mar 31, 2:52 PM · Restricted Project
lebedev.ri added a comment to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

Hello.
2 points:

Tue, Mar 31, 2:17 PM · Restricted Project
lebedev.ri added inline comments to D76974: [Attributor] Make attributor aware of aligned_alloc for heap to stack conversion.
Tue, Mar 31, 11:26 AM · Restricted Project
lebedev.ri added a comment to D77144: [NFC] Remove waymarking because it improves performances.

Why did instructions_count metric change?

Tue, Mar 31, 8:16 AM · Restricted Project
lebedev.ri updated the diff for D76921: [llvm-exegesis] 'Min' repetition mode.

Forego of llvm::make_scope_exit in favor of more manual approach.

Tue, Mar 31, 6:37 AM · Restricted Project
lebedev.ri added inline comments to D76921: [llvm-exegesis] 'Min' repetition mode.
Tue, Mar 31, 6:37 AM · Restricted Project
lebedev.ri updated the diff for D76921: [llvm-exegesis] 'Min' repetition mode.

@gchatelet @courbet
Thank you for taking a look!
Addressed review notes.

Tue, Mar 31, 1:37 AM · Restricted Project

Mon, Mar 30

lebedev.ri added inline comments to D76956: [TTI][SLP] Add TTI interface to estimate cost of chain of vector inserts/extracts..
Mon, Mar 30, 11:58 PM · Restricted Project
lebedev.ri added inline comments to D76921: [llvm-exegesis] 'Min' repetition mode.
Mon, Mar 30, 11:56 AM · Restricted Project
lebedev.ri added inline comments to D76921: [llvm-exegesis] 'Min' repetition mode.
Mon, Mar 30, 11:56 AM · Restricted Project

Sat, Mar 28

lebedev.ri added a comment to D76983: [InstCombine] Transform extractelement-trunc -> bitcast-extractelement.

Should we be checking anything about legality of the types here?

Sat, Mar 28, 8:36 AM · Restricted Project
lebedev.ri added a comment to D74156: [llvm-exegesis] Exploring X86::OperandType::OPERAND_COND_CODE.

... did that explanation of the question i'm having made any sense?

Thx for digging in the conversation !
Ok it makes more sense now.

Awesome! :)

I discussed it a bit with @courbet:

  • We want the analysis tool to stay simple so we'd rather not make it knowledgeable of the repetition mode.
  • We'd like to still be able to select either repetition mode to dig into special cases

    So we could add a third min repetition mode that would run both and take the minimum. It could be the default option. Would you have some time to look what it would take to add this third mode?

If that is the preferred direction then i will try to take a look :)
Thank you for your patience.

Sat, Mar 28, 5:55 AM · Restricted Project
lebedev.ri added inline comments to D75425: [docs] Added solutions to slow build under common problems.
Sat, Mar 28, 2:42 AM · Restricted Project

Fri, Mar 27

lebedev.ri added a comment to D76886: [InlineFunction] Disable emission of alignment assumptions by default.

Is there no phase-ordering-esque test for that?

After looking at IR diffs a bit more, one difference I noticed is that jump threading is not being performed in some places. In this case the reason is not multi-use, but cost heuristics based on instruction counts. The alignment assumption adds 4 extra instructions, and if the block duplication threshold is at 6 instructions, that can easily make a difference. The phase ordering test is loosely based around that idea. Does that look useful to you?

Yes, thank you.

Fri, Mar 27, 7:37 AM · Restricted Project
lebedev.ri added inline comments to D76886: [InlineFunction] Disable emission of alignment assumptions by default.
Fri, Mar 27, 7:37 AM · Restricted Project
lebedev.ri updated the summary of D76921: [llvm-exegesis] 'Min' repetition mode.
Fri, Mar 27, 7:03 AM · Restricted Project
lebedev.ri updated the summary of D76921: [llvm-exegesis] 'Min' repetition mode.
Fri, Mar 27, 7:02 AM · Restricted Project
lebedev.ri created D76921: [llvm-exegesis] 'Min' repetition mode.
Fri, Mar 27, 7:02 AM · Restricted Project

Thu, Mar 26

lebedev.ri added a comment to D76886: [InlineFunction] Disable emission of alignment assumptions by default.

Is there no phase-ordering-esque test for that?

Thu, Mar 26, 2:08 PM · Restricted Project
lebedev.ri added a comment to D76871: Expose `attributor-disable` to the new and old pass managers.

I think the patch description is lacking - the switch already exists, and it works.
There should be an explanation as to why it is being moved.

Thu, Mar 26, 1:36 PM · Restricted Project
lebedev.ri accepted D76844: [InstCombine] try to reduce shuffle with bitcasted operand.

This seems reasonable to me, unless @RKSimon has concerns.

Thu, Mar 26, 12:29 PM · Restricted Project
lebedev.ri accepted D76727: [VectorCombine] transform bitcasted shuffle to narrower elements.

LG

Thu, Mar 26, 12:29 PM · Restricted Project
lebedev.ri added a comment to D76623: [VectorCombine] try to form a better extractelement.

While this looks good in principle, i'm starting to have second thoughts about the general design here..

Thu, Mar 26, 12:29 PM · Restricted Project
lebedev.ri accepted D76586: [Utils][FIX] Properly deal with occasionally deleted functions.

Seems reasonable to me.

Thu, Mar 26, 11:55 AM · Restricted Project
lebedev.ri added a comment to D76674: [Attributor] Derive better alignment for accessed pointers.

Why can't we use ABI info without known access?
I mean, is it possible to use ABI info in initialize?

Thu, Mar 26, 10:50 AM · Restricted Project
lebedev.ri requested changes to D76434: [SCEV] Query for immediate cost in Expander.

<...>
Thus i'm asking, what does getIntImmCost() model? rthroughput or size?

Thu, Mar 26, 8:06 AM
lebedev.ri added a comment to D75815: [InstCombine] Simplify calls with "returned" attribute.

Maybe we should add new GCC-like attribute “noipa” to disable these optimizations?

Thu, Mar 26, 7:33 AM · Restricted Project
lebedev.ri updated subscribers of D75815: [InstCombine] Simplify calls with "returned" attribute.

Yeah, like i said, noinline is insufficient here, and i don't actually
think we currently have the right tool to block any such transform:

Do you think it makes sense to check for noinline before transforming the call into the argument, i.e.:

if (!Call.use_empty() && !Call.isMustTailCall()) {
  Function *Fn = dyn_cast<Function>(Callee);
  if (Fn && Fn->hasFnAttribute(Attribute::NoInline))
    if (Value *ReturnedArg = Call.getReturnedArgOperand())
      return replaceInstUsesWith(Call, ReturnedArg);
}

I can only once again point at

Thu, Mar 26, 7:33 AM · Restricted Project
lebedev.ri added a comment to D75815: [InstCombine] Simplify calls with "returned" attribute.

I'm going to guess __attribute__((no_sanitize())) needs to also imply (read: implicitly add) noinline attribute,
but even that is not sufficient to block inter-procedural optimizations.
See also disscussion in https://reviews.llvm.org/D53431

I don't think noinline works here:
<...>

Thu, Mar 26, 4:17 AM · Restricted Project

Wed, Mar 25

lebedev.ri accepted D76800: [InstCombine] Fix Incorrect fold of ashr+xor -> lshr w/ vectors.

LG, thank you.

Wed, Mar 25, 11:58 PM · Restricted Project
lebedev.ri added a comment to D76800: [InstCombine] Fix Incorrect fold of ashr+xor -> lshr w/ vectors.

static inline Constant *getSafeVectorConstantForBinop()

Yes, i think that's the one.

Wed, Mar 25, 3:11 PM · Restricted Project
lebedev.ri requested changes to D76800: [InstCombine] Fix Incorrect fold of ashr+xor -> lshr w/ vectors.

What we need to do here is "define" undef to be 0.

----------------------------------------
define <4 x i5> @test_v4i5_not_ashr_negative_const_undef(<4 x i5> %a0) {
%0:
  %1 = ashr <4 x i5> { 29, 27, undef, 23 }, %a0
  %2 = xor <4 x i5> { 31, 31, 31, undef }, %1
  ret <4 x i5> %2
}
=>
define <4 x i5> @test_v4i5_not_ashr_negative_const_undef(<4 x i5> %a0) {
%0:
  %1 = lshr <4 x i5> { 2, 4, 0, 8 }, %a0
  ret <4 x i5> %1
}
Transformation seems to be correct!
Wed, Mar 25, 2:38 PM · Restricted Project
lebedev.ri added a comment to D74918: Add method to TargetInfo to get CPU cache line size.

Like i said in previous comments, this can be moved later, that is not a blocker.
Ship it first :)

Wed, Mar 25, 9:43 AM · Restricted Project
lebedev.ri added a comment to D76727: [VectorCombine] transform bitcasted shuffle to narrower elements.

Eeeek.
I'm not sure what i was thinking, there is indeed no point in modelling the cost
of bitcast here because we have the exact same bitcast instruction in either case.

Wed, Mar 25, 6:59 AM · Restricted Project
lebedev.ri added a comment to D75815: [InstCombine] Simplify calls with "returned" attribute.

I'm going to guess __attribute__((no_sanitize())) needs to also imply (read: implicitly add) noinline attribute,
but even that is not sufficient to block inter-procedural optimizations.
See also disscussion in https://reviews.llvm.org/D53431

Wed, Mar 25, 4:17 AM · Restricted Project

Tue, Mar 24

lebedev.ri added a comment to D76673: [Attributor][FIX] Prevent alignment breakage wrt. must-tail calls.

Thanks, this looks about reasonable to me, although i'm not familiar with this code to fully review it.

Tue, Mar 24, 11:57 PM · Restricted Project
lebedev.ri added a reviewer for D76727: [VectorCombine] transform bitcasted shuffle to narrower elements: t.p.northover.

From reading through the getCastInstrCost()'s i don't think any backend
currently models it, but there's this comment in AArch64ISelLowering.cpp

namespace llvm {
Tue, Mar 24, 11:25 PM · Restricted Project
lebedev.ri added a comment to D76727: [VectorCombine] transform bitcasted shuffle to narrower elements.

Do we need to count the cost of bitcasts too?

Tue, Mar 24, 1:28 PM · Restricted Project
lebedev.ri added inline comments to D76707: [DAGCombine] Add basic optimizations for FREEZE in SelDag.
Tue, Mar 24, 10:12 AM · Restricted Project
lebedev.ri added a comment to D76673: [Attributor][FIX] Prevent alignment breakage wrt. must-tail calls.

How do we know this is only about alignment?

Tue, Mar 24, 1:35 AM · Restricted Project
lebedev.ri accepted D76674: [Attributor] Derive better alignment for accessed pointers.

This looks good to me, thanks for relieving me from having to upstream this myself :)

Tue, Mar 24, 1:35 AM · Restricted Project
lebedev.ri added a comment to D76674: [Attributor] Derive better alignment for accessed pointers.

That's the patch i showed, yes :)
I'm not sure this is sound.
Given underaligned load, we can't just take max(align of load, abi alignment)

Hm, so we can only look at the abi if the load alignment is unspecified?

Tue, Mar 24, 1:35 AM · Restricted Project
lebedev.ri added a comment to D76674: [Attributor] Derive better alignment for accessed pointers.

That's the patch i showed, yes :)
I'm not sure this is sound.
Given underaligned load, we can't just take max(align of load, abi alignment)

Tue, Mar 24, 1:03 AM · Restricted Project
lebedev.ri added a comment to D76665: [asan] Stop instrumenting NetBSD-specific link_set sections.

This is probably missing a test

Tue, Mar 24, 12:30 AM · Restricted Project, Restricted Project

Mon, Mar 23

lebedev.ri added a comment to D76622: [analyzer] ConstraintManager - use EXPENSIVE_CHECKS instead of (gcc specific) __OPTIMIZE__ guard.

SGTM

Mon, Mar 23, 10:21 AM · Restricted Project
lebedev.ri added a comment to D74918: Add method to TargetInfo to get CPU cache line size.

@lebedev.ri Where should I put this? Could you maybe point me to another LLVM target API that clang uses that I could base this off? Maybe it should go inside the triple or just be a static function?

Mon, Mar 23, 10:21 AM · Restricted Project
lebedev.ri added a comment to D75591: [OpenMP] Add firstprivate as a default data-sharing attribute to clang.

Did you add the warning @lebedev.ri mentioned? (@lebedev.ri I assume/hope a warning is sufficient here?)

Mon, Mar 23, 12:31 AM · Restricted Project, Restricted Project

Sun, Mar 22

lebedev.ri added inline comments to D76550: [Attributor] Improve the alignment of the loads.
Sun, Mar 22, 2:28 PM · Restricted Project
lebedev.ri added a comment to D76572: Replace `T(x)` with `reinterpret_cast<T>(x)` everywhere it means reinterpret_cast. No functional change.

Passing-by remark:

I wrote a Clang warning [not pictured] to diagnose any use of T(x) which was not equivalent to static_cast<T>(x).

I'm not sure whether or not this will pass the bar for a clang diagnostic,
but that does sound like a good clang-tidy readability check.

Sun, Mar 22, 1:55 PM · Restricted Project
lebedev.ri added a reviewer for D76550: [Attributor] Improve the alignment of the loads: courbet.

@courbet To be honest

explicit MaybeAlign(uint64_t Value) {
  assert((Value == 0 || llvm::isPowerOf2_64(Value)) &&
         "Alignment is neither 0 nor a power of 2");
  if (Value)
    emplace(Value);
}

that if-check looks like a major rake.

Sun, Mar 22, 12:18 PM · Restricted Project
lebedev.ri added inline comments to D76140: [InlineFunction] update attributes during inlining.
Sun, Mar 22, 11:44 AM · Restricted Project, Restricted Project
lebedev.ri accepted D76508: [VectorUtils] move x86's scaleShuffleMask to generic VectorUtils.

Thanks, that helped convince me of this not being endian-specific.
LG

Sun, Mar 22, 10:40 AM · Restricted Project
lebedev.ri added inline comments to D76140: [InlineFunction] update attributes during inlining.
Sun, Mar 22, 10:08 AM · Restricted Project, Restricted Project