Page MenuHomePhabricator

hfinkel (Hal Finkel)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 16 2012, 6:02 PM (330 w, 5 d)

Recent Activity

Today

hfinkel added a comment to D59657: [LangRef] Clarify codegen expectations for intrinsics with fp/integer-only overloads.

The audience of this are people who are defining new target intrinsics.

Thu, Mar 21, 1:36 PM · Restricted Project
hfinkel added a comment to D59657: [LangRef] Clarify codegen expectations for intrinsics with fp/integer-only overloads.

I don't understand what this is supposed to mean to who the audience of this statement is?

Thu, Mar 21, 11:48 AM · Restricted Project

Yesterday

hfinkel accepted D59623: [PPC] Refactor PPCBranchSelector.cpp.

LGTM.

-eric

Wed, Mar 20, 7:14 PM · Restricted Project

Tue, Mar 19

hfinkel added a comment to D47849: [OpenMP][Clang][NVPTX] Enable math functions called in an OpenMP NVPTX target device region to be resolved as device-native function calls.

We need to make progress on this, and I'd like to suggest a path forward...

Tue, Mar 19, 6:11 PM · Restricted Project

Fri, Mar 15

hfinkel accepted D59437: Remove immarg from llvm.expect.

LGTM. Maybe we can also add the test case from the PR as one of Clang's regression tests?

Fri, Mar 15, 3:40 PM

Thu, Mar 14

hfinkel added a comment to D59065: [BasicAA] Simplify inttoptr(and(ptrtoint(X), C)) to X, if C preserves all significant bits..

I'm really afraid that this isn't sound. We've had a number of issues in this space, and we've always resisted attempts to make BasicAA look through inttoptr/ptrtotint. Once you convert to an integer, control dependencies can effectively add additional underlying objects. In cases where this is sound, why not fold away the whole inttoptr(and(ptrtoint)) in the first place?

Thu, Mar 14, 9:32 AM · Restricted Project
hfinkel accepted D59294: [TableGen] Give meaningful msg for def use in multiclass.

LGTM.

Thu, Mar 14, 8:55 AM
hfinkel accepted D59247: [TableGen] Let list elements have a trailing comma.

LGTM

Thu, Mar 14, 8:55 AM

Thu, Mar 7

hfinkel accepted D59011: [PowerPC] Remove the override of isMachineVerifierClean() to open machine verifier.

Thanks to everyone who worked on this! LGTM.

Thu, Mar 7, 3:50 AM · Restricted Project

Tue, Mar 5

hfinkel accepted D58895: [TableGen] Allow lists to be concatenated through '#'.

Awesome :)
LGTM

Tue, Mar 5, 8:54 AM · Restricted Project

Mon, Mar 4

hfinkel added inline comments to D58895: [TableGen] Allow lists to be concatenated through '#'.
Mon, Mar 4, 10:15 AM · Restricted Project

Thu, Feb 28

hfinkel accepted D58775: [Tablegen] Add support for the !mul operator..

Cool,. I've wanted this for a while. LGTM.

Thu, Feb 28, 10:13 AM · Restricted Project

Tue, Feb 26

hfinkel added inline comments to D56772: [MIR] Add simple PRE pass to MachineCSE.
Tue, Feb 26, 12:03 PM

Mon, Feb 25

hfinkel accepted D56286: [OPENMP] Deal with additional store inserted by Clang under -fno-PIC for PowerPC..

Is this STW instruction something that instruction scheduling might move? (@nemanjai ?)

Mon, Feb 25, 10:27 AM · Restricted Project, Restricted Project

Feb 12 2019

hfinkel accepted D58128: [PowerPC] Stop defining _ARCH_PWR6X on POWER7 and up.

LGTM

Feb 12 2019, 9:24 AM · Restricted Project

Feb 7 2019

hfinkel accepted D57215: [CodeExtractor] Update function's assumption cache after extracting blocks from it.
Feb 7 2019, 8:14 PM · Restricted Project
hfinkel added inline comments to D57215: [CodeExtractor] Update function's assumption cache after extracting blocks from it.
Feb 7 2019, 12:38 PM · Restricted Project

Feb 6 2019

hfinkel added inline comments to D57215: [CodeExtractor] Update function's assumption cache after extracting blocks from it.
Feb 6 2019, 7:03 PM · Restricted Project
hfinkel added inline comments to D57215: [CodeExtractor] Update function's assumption cache after extracting blocks from it.
Feb 6 2019, 5:34 PM · Restricted Project

Feb 5 2019

hfinkel added a comment to D57759: [PowerPC] Code Cleanup Remove u1imm.

Hi Hal,
Thank you for the review.
I've looked around but the only place we use u1imm is where we only have 1 bit to use in the instruction anyway so we have no way to represent -1 anyway.

Feb 5 2019, 2:56 PM
hfinkel accepted D57331: [PowerPC] Fix printing of negative offsets in call instruction dissasembly..

LGTM

Feb 5 2019, 2:54 PM · Restricted Project
hfinkel accepted D57759: [PowerPC] Code Cleanup Remove u1imm.

LGTM. I suppose that I thought that this was needed at some point to parse (1, 0) instead of (-1, 0), but if this is not needed in practice, then it should be simplified.

Feb 5 2019, 10:41 AM

Feb 4 2019

hfinkel added a comment to D57542: [WarnMissedTransforms] Do not warn about already vectorized loops..

LGTM.

Feb 4 2019, 9:55 AM · Restricted Project
hfinkel accepted D57542: [WarnMissedTransforms] Do not warn about already vectorized loops..

LGTM.

Feb 4 2019, 9:53 AM · Restricted Project

Jan 30 2019

hfinkel accepted D57439: [PowerPC] delete no more needed workaround for readsRegister() in PowerPC.

LGTM

Jan 30 2019, 10:19 AM

Jan 28 2019

hfinkel accepted D57368: [PowerPC] [NFC] Create a helper function to copy register to particular register class at PPCFastISel.

LGTM

Jan 28 2019, 11:01 PM
hfinkel added inline comments to D53184: [LangRef] Clarify semantics of volatile operations..
Jan 28 2019, 12:42 PM
hfinkel added inline comments to D53184: [LangRef] Clarify semantics of volatile operations..
Jan 28 2019, 12:27 PM

Jan 26 2019

hfinkel accepted D56812: [LoopReroll] Fix reroll root legality checking..

LGTM

Jan 26 2019, 7:46 AM · Restricted Project

Jan 25 2019

hfinkel added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

Agreed, but it might be worthwhile to choose a guinea pig (openmp) first. I find it difficult to think of all the possible consequences of changes like this.

I feel it's likely there are some things we're not considering, but as a result, the prudent thing to do is to propose the change for libc++ also. We'll, potentially, get a much-wider set of feedback and, based on that, be able to make a better-informed decision. That's the path I prefer that we take. I recommend that you write an RFC for cfe-dev proposing to apply this change to all runtime libraries (openmp, libc++, etc.). It might turn out that there are technical reasons why this would not work for some libraries and would work for others, but I think that we should understand what those reasons are before we proceed. Once we've collected this feedback, the ordering in which we try applying the change to specific projects can be determined. libomp can then be the guinea pig if that's the logical thing to do.

Sounds reasonable.

I'd like to bake this patch a little longer to apply some of the ideas we're discussing. I'll then post the RFC and point here before working on patches for other runtimes.

cfe-dev is sufficient for the RFC?

Jan 25 2019, 10:09 AM · Restricted Project
hfinkel added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

Agreed, but it might be worthwhile to choose a guinea pig (openmp) first. I find it difficult to think of all the possible consequences of changes like this.

Jan 25 2019, 9:47 AM · Restricted Project

Jan 24 2019

hfinkel added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

Jan 24 2019, 5:39 PM · Restricted Project
hfinkel accepted D54653: [IRBuilder] Remove positivity check from CreateAlignmentAssumption().

Took over as discussed, rewrote from scratch based on previous disscussions/review feedback.

@hfinkel et al - please formally accept if this makes sense.

Jan 24 2019, 11:30 AM
hfinkel accepted D55758: [TableGen] : Extend !if semantics through new language feature !ifs.

LGTM

Jan 24 2019, 10:46 AM
hfinkel added a comment to D56772: [MIR] Add simple PRE pass to MachineCSE.

Have you measured the compile-time impact?

Jan 24 2019, 9:52 AM
hfinkel added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

I think this patch would effectively lose the ability to use a custom standalone build of the OpenMP runtime: When hacking on the runtime it's pretty convenient to only build and install the openmp repository and use environment variables to switch between different versions. When putting libomp.so into lib/clang/9.0.0/lib/linux/x86_64 [1] this path will be very first one that Clang instructs the linker to look at. As a result the "custom" location that I specify in LIBRARY_PATH won't be honored and I'll always end up using the version installed next to the compiler.

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

1: This path doesn't exist in a current installation of trunk. Is any other project using this path in its default settings? Sorry, I haven't been following LLVM activities pretty closely for the last months.

Jan 24 2019, 9:33 AM · Restricted Project
hfinkel accepted D57078: [PowerPC] Enhance the fast selection of cmp instruction and clean up related asserts.

LGTM

Jan 24 2019, 8:56 AM

Jan 23 2019

hfinkel added inline comments to D55725: [OpenMP] Add libs to clang-dedicated directories.
Jan 23 2019, 7:48 PM · Restricted Project
hfinkel added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

In general, this makes sense to me.

Jan 23 2019, 2:43 PM · Restricted Project
hfinkel added a comment to D55373: [LSR] Generate formulae to enable more indexed accesses.

ping

Jan 23 2019, 9:12 AM · Restricted Project

Jan 22 2019

hfinkel added a comment to D56772: [MIR] Add simple PRE pass to MachineCSE.

Ping!

Jan 22 2019, 4:58 PM
hfinkel accepted D53966: Codegen support for atomicrmw fadd/fsub.

ping

Jan 22 2019, 5:34 AM

Jan 21 2019

hfinkel accepted D53965: IR: Add fp operations to atomicrmw.

LGTM

Jan 21 2019, 5:56 PM
hfinkel accepted D57040: [Analysis] Fix isSafeToLoadUnconditionally handling of volatile..

LGTM

Jan 21 2019, 5:52 PM
hfinkel accepted D53184: [LangRef] Clarify semantics of volatile operations..

Ping

Jan 21 2019, 4:21 PM

Jan 17 2019

hfinkel accepted D56838: [CGP] Check for existing inttotpr before creating new one.

Can we run EarlyCSE after CGP (instead of trying to do these kinds of point fixes)?

Hi Hal,

Thank you for looking into this!

That's a great suggestion! Originally I just eyeballed it as too expensive compile-time wise for the little effect that is achieved here.

This time around with you pointing it out, I performed the actual measurements for a very large suite of shaders (the downstream target in question is a GPU). I see about 1.0 (+/-0.3)% increase in overall compile time (the part of it that happens at an application run-time) on average. The generated code quality improvement that comes out of this may be important, but it doesn't trigger often enough to justify 1% compile time increase across the board.

Thanks,
Roman

Jan 17 2019, 7:27 PM
hfinkel added a comment to D56867: [ValueTracking] Early out of isValidAssumeForContext if the assume and the context instruction are the same.

If we're going to do anything here, I'd prefer to change it to true, since that's the right answer... it seems like it isn't doing any harm in the meantime.

Jan 17 2019, 3:53 PM
hfinkel accepted D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions.

Please, in the future, make sure you post full-context patches.

Jan 17 2019, 9:43 AM · Restricted Project
hfinkel added a comment to D56838: [CGP] Check for existing inttotpr before creating new one.

Can we run EarlyCSE after CGP (instead of trying to do these kinds of point fixes)?

Jan 17 2019, 7:30 AM

Jan 11 2019

hfinkel accepted D56551: [LoopVectorizer] give more advice in remark about failure to vectorize call.

LGTM

Jan 11 2019, 8:38 AM

Jan 10 2019

hfinkel added inline comments to D56551: [LoopVectorizer] give more advice in remark about failure to vectorize call.
Jan 10 2019, 6:05 PM

Jan 9 2019

hfinkel accepted D56486: [CodeGen] Ignore return sext/zext attributes of unused results for tail calls.

LGTM

Jan 9 2019, 9:20 AM

Jan 7 2019

hfinkel accepted D55686: [PowerPC] Fix assert from machine verify pass that unmatched register class about fcmp selection in fast-isel.
Jan 7 2019, 9:42 AM
hfinkel accepted D56362: [DemandedBits] Use SetVector for Worklist.

LGTM

Jan 7 2019, 9:22 AM

Jan 4 2019

hfinkel added inline comments to D55758: [TableGen] : Extend !if semantics through new language feature !ifs.
Jan 4 2019, 1:22 PM
hfinkel accepted D56247: [BDCE] Remove dead uses of arguments.

LGTM

Jan 4 2019, 10:52 AM
hfinkel added a comment to D38501: [ValueTracking] Fix a misuse of APInt in GetPointerBaseWithConstantOffset.

Thanks! Do you need someone to commit the change on your behalf?

Jan 4 2019, 5:57 AM

Jan 2 2019

hfinkel committed rL350220: [BasicAA] Support arbitrary pointer sizes (and fix an overflow bug).
[BasicAA] Support arbitrary pointer sizes (and fix an overflow bug)
Jan 2 2019, 8:31 AM
hfinkel closed D38662: [BasicAA] Support arbitrary pointer sizes (and fix an overflow bug).
Jan 2 2019, 8:31 AM

Jan 1 2019

hfinkel accepted D56185: [BDCE] Remove instructions without demanded bits.

LGTM

Jan 1 2019, 10:03 AM

Dec 31 2018

hfinkel added a comment to D55563: [BDCE][DemandedBits] Detect dead uses of undead instructions.

Fix removal of dead uses. The previous version had a check whether the previous demanded bits are empty and only dropped the use from the DeadUses set in that case. The motivation for this was that the use could only be part of DeadUses if the demanded bits are empty, so we can save a redundant hash lookup. Of course, this is not actually true, because the previous demanded bits are for the instruction across all users, which is a superset of the demanded bits for a single use. (That being kind of the whole point of this change, I'm really not sure what I was thinking there...)

So now I'm just always doing the erase() call when a use has non-empty demanded bits. I've also added a testcase that was miscompiled with the previous version. I believe that this is also what caused the failure in enc-3des from the test-suite.

Dec 31 2018, 9:00 AM

Dec 30 2018

hfinkel added inline comments to D42590: [PowerPC] Try to move the stack pointer update instruction later in the prologue and earlier in the epilogue (Version 2).
Dec 30 2018, 5:21 PM · Restricted Project
hfinkel added inline comments to D42590: [PowerPC] Try to move the stack pointer update instruction later in the prologue and earlier in the epilogue (Version 2).
Dec 30 2018, 8:58 AM · Restricted Project
hfinkel added inline comments to D56156: [DAGCombiner][X86][PowerPC] Teach visitSIGN_EXTEND_INREG to fold (sext_in_reg (aext/sext x)) -> (sext x) when x has more than 1 sign bit and the sext_inreg is from one of them..
Dec 30 2018, 7:32 AM

Dec 29 2018

hfinkel added a comment to D56148: [PowerPC] Fix machine verify pass error for PATCHPOINT pseudo instruction that bad machine code.

So this rell-request

Dec 29 2018, 8:35 AM

Dec 28 2018

hfinkel accepted D56119: [PowerPC] Fix ADDE, SUBE do not know how to promote operator.
Dec 28 2018, 6:37 AM

Dec 27 2018

hfinkel accepted D49076: [PowerPC] handle ISD:TRUNCATE in BitPermutationSelector.

LGTM

Dec 27 2018, 10:14 PM
hfinkel accepted D55806: [PowerPC] fix register class after converting X-FORM instruction to D-FORM instruction.

LGTM

Dec 27 2018, 3:39 PM
hfinkel accepted D56078: [PowerPC] Remove the implicit use of the register if it is replaced by Imm.

LGTM

Dec 27 2018, 9:23 AM
hfinkel added inline comments to D55686: [PowerPC] Fix assert from machine verify pass that unmatched register class about fcmp selection in fast-isel.
Dec 27 2018, 9:12 AM
hfinkel accepted D56077: [PowerPC] Fix assert from machine verify pass that atomic pseudo expanding causes mismatched register class.

Please fixup the comments, but LGTM.

Dec 27 2018, 9:09 AM

Dec 21 2018

hfinkel added a comment to D54498: AbstractCallSite -- A unified interface for (in)direct and callback calls.

I can certainly split this into more patches if that is preferred. I'm unsure though how to test the metadata and abstract call sites without a user.

Dec 21 2018, 2:21 PM
hfinkel added a comment to D54498: AbstractCallSite -- A unified interface for (in)direct and callback calls.

A high level strategy comment. I think you're trying to go a bit too far in one patch. I'd advise first writing a patch which does nothing but implement the IPConstProp adjustments "the hard way" (i.e. without the AbstractCallSite) and focus on defining the metadata with the semantics you once. To really justify the complexity of the abstraction, you need more than one user. You could add another user to this patch, but doing so will just slow down the review. Separating this into a series of patches (IPConstProp w/metadata def, second user, then introduce abstraction) will make it much easier to see the necessity and make each piece more easily reviewable.

Dec 21 2018, 10:36 AM
hfinkel added a comment to D55977: [PowerPC] Fix the bug of ISD::ADDE to set its second return type to glue.

LGTM

Dec 21 2018, 9:37 AM
hfinkel accepted D55977: [PowerPC] Fix the bug of ISD::ADDE to set its second return type to glue.

LGTM

Dec 21 2018, 9:35 AM
hfinkel accepted D55996: [PowerPC] Fix CR Bit spill pseudo expansion.

LGTM. This certainly looks like a better solution; the previous code was indeed trying to make sure that the containing CR field was defined before the mfocrf. Thanks!

Dec 21 2018, 8:39 AM

Dec 20 2018

hfinkel accepted D55969: [BasicAA] Fix AA bug on dynamic allocas and stackrestore.

LGTM

Dec 20 2018, 7:19 PM
hfinkel added a comment to D49084: FileCheck: Add support for numeric variables and expressions.

Thanks for updating this patch. It will be very useful!

Dec 20 2018, 7:13 PM · Restricted Project
hfinkel added inline comments to D55928: [OpenMP] Add flag for preventing the extension to 64 bits for the collapse loop counter.
Dec 20 2018, 8:26 AM

Dec 19 2018

hfinkel accepted D38662: [BasicAA] Support arbitrary pointer sizes (and fix an overflow bug).

hey @efriedma - @hfinkel and I are wondering if you have any comments on this updated revision of BasicAA improvements. Thanks!

Dec 19 2018, 12:43 PM
hfinkel accepted D52116: Introduce llvm.loop.parallel_accesses and llvm.access.group metadata..

Aside from the requested renaming (see below), this LGTM.

Dec 19 2018, 12:38 PM
hfinkel accepted D55563: [BDCE][DemandedBits] Detect dead uses of undead instructions.
Dec 19 2018, 8:11 AM
hfinkel accepted D55754: [PowerPC] Implement the ”isSelectSupported()“ target hook.

LGTM too.

Dec 19 2018, 7:39 AM

Dec 18 2018

hfinkel accepted D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata..

Minor typo noted below, but otherwise, LGTM (to avoid any misunderstanding: this should be committed after the LLVM change lands).

Dec 18 2018, 5:38 PM
hfinkel added inline comments to D52116: Introduce llvm.loop.parallel_accesses and llvm.access.group metadata..
Dec 18 2018, 5:32 PM
hfinkel added a comment to D53184: [LangRef] Clarify semantics of volatile operations..

Yes, volatile loads and stores access the "same" state as inaccessiblememonly functions, I think.

Dec 18 2018, 4:42 PM
hfinkel added inline comments to D55758: [TableGen] : Extend !if semantics through new language feature !ifs.
Dec 18 2018, 4:10 PM
hfinkel added inline comments to D55563: [BDCE][DemandedBits] Detect dead uses of undead instructions.
Dec 18 2018, 4:03 PM
hfinkel added a comment to D55461: [PowerPC] Update Vector Costs for P9.

Also, cost model changes can be tested directly. See tests in test/Analysis/CostModel/PowerPC/

Dec 18 2018, 3:45 PM
hfinkel added a comment to D55461: [PowerPC] Update Vector Costs for P9.

The calculation the loop vectorizer is making is comparing vector cost / vector factor to scalar cost. The calculation the SLP vectorizer is making is comparing vector cost to scalar cost * vector factor. In either case the vector factor is involved, and once the vector factor is involved it is really a throughput calculation and not a latency calculation. I don't think vectorization can make sense as a latency calculation - the scalar ops are always going to be at least as cheap. The win is in doing more at the same time, not in delivering a particular result faster.

Thanks for explaining. I am not familiar with SLP enough, maybe @hfinkel can help to see whether it is reasonable to consider execution unit num in vectorizer's cost model. Thanks!

Dec 18 2018, 3:45 PM
hfinkel added a comment to D53184: [LangRef] Clarify semantics of volatile operations..

I can add a sentence to specifically state that this can modify state accessed by functions marked with the inaccessiblememonly attribute, and vice versa, if that's the interaction you're worried about. (I don't think it's practical to have multiple forms of inaccessible state.)

Dec 18 2018, 3:02 PM
hfinkel added a comment to D54742: [CodeMetrics] Don't let extends of i1 be free..
Dec 18 2018, 1:14 PM

Dec 17 2018

hfinkel accepted D55785: [LoopVectorize] Rename pass options. NFC..

LGTM. Thanks, I think this naming is more accurate and useful.

Dec 17 2018, 12:54 PM
hfinkel accepted D55716: [LoopUnroll] Honor '#pragma unroll' even with -fno-unroll-loops..

LGTM

Dec 17 2018, 12:17 PM
hfinkel added inline comments to D55710: add pragmas to control Software Pipelining optimisation.
Dec 17 2018, 11:27 AM
hfinkel added a comment to D55716: [LoopUnroll] Honor '#pragma unroll' even with -fno-unroll-loops..

Such explicit transformations should take precedence over disabling heuristics.

Dec 17 2018, 8:48 AM

Dec 14 2018

hfinkel added a reviewer for D55710: add pragmas to control Software Pipelining optimisation: Meinersbur.
Dec 14 2018, 10:17 AM
hfinkel accepted D55666: [Transforms] Preserve metadata when converting invoke to call..

I think that this is okay. There's no semantic change nor a change in control dependence. For this to cause a problem, there would need to be metadata that would change meaning or validity when the instruction changes from an invoke to a call (where the invoke is semantically equivalent to the call).

Dec 14 2018, 8:20 AM
hfinkel accepted D55690: [TransformWarning] Do not warn missed transformations in optnone functions..

LGTM

Dec 14 2018, 8:10 AM

Dec 13 2018

hfinkel added inline comments to D55666: [Transforms] Preserve metadata when converting invoke to call..
Dec 13 2018, 8:12 PM