rengolin (Renato Golin)
Toolchain Engineer

Projects

User does not belong to any projects.

User Details

User Since
Oct 19 2012, 12:57 AM (268 w, 5 d)

Recent Activity

Wed, Dec 6

rengolin added a comment to D40362: [TableGen][AsmMatcherEmitter] Only choose specific diagnostic for enabled instruction.

I'd wait someone from Mips to review the patch before committing, as the messages change a lot and I have no idea how valid they are.

Wed, Dec 6, 1:58 AM
rengolin added a comment to D40361: [AArch64][SVE] Asm: Add ZIP1/ZIP2 instructions (predicate/data vectors).

There are too many redundant tests. I'd keep one for each size/type on both boundaries (0, 31) and add negative tests to check that you can't zip predicate registers with data registers, register numbers out of bounds, different number of registers (2, 4), etc.

Wed, Dec 6, 1:56 AM
rengolin added a comment to D28975: [LV] Introducing VPlan to model the vectorized code and drive its transformation.

Hi Ayal,

Wed, Dec 6, 1:18 AM

Sun, Nov 26

rengolin requested changes to D39910: [ARM] Issue an eror when non-general-purpose registers used in address operands (alternative).

I think this change goes against the recent trend of printing multiple diagnostics, instead of a huge dump that will certainly be inaccurate at least some times.

Sun, Nov 26, 3:51 AM

Wed, Nov 22

rengolin accepted D39346: [LV] [ScalarEvolution] Fix PR34965 - Cache pointer stride information before LV code gen.

Hi Diego,

Wed, Nov 22, 10:25 AM

Tue, Nov 21

rengolin accepted D39196: [ARM] Remove pre-UAL FLDM/FSTM aliases.

LGTM, thanks!

Tue, Nov 21, 8:11 AM
rengolin accepted D39193: [ARM] Don't omit non-default predication code.

Ok, with the updated comment, LGTM. Thanks!

Tue, Nov 21, 7:28 AM
rengolin accepted D36746: [Asm] Improve "too few operands" errors.

LGTM, thanks!

Tue, Nov 21, 5:24 AM
rengolin accepted D36744: [Asm] Finish matching once end of formal and actual lists reached (NFC).

LGTM, thanks!

Tue, Nov 21, 5:24 AM
rengolin added inline comments to D39193: [ARM] Don't omit non-default predication code.
Tue, Nov 21, 5:19 AM
rengolin accepted D39195: [ARM] Add diagnostics for SPR/DPR lists.

Sorry, fell through. No brainer, LGTM. Thanks!

Tue, Nov 21, 5:04 AM

Mon, Nov 20

rengolin added a comment to D40256: [ARM] disable FPU features when using soft floating point..

Wasn't that the mess about -mfpu=softfp?

Mon, Nov 20, 1:22 PM

Sun, Nov 19

rengolin accepted D38676: [LV] Model masking in VPlan, introducing VPInstructions.

Great, LGTM now, thanks!

Sun, Nov 19, 9:49 AM

Sat, Nov 18

rengolin accepted D40030: [AArch64][TableGen] Skip tied result operands for InstAlias.

LGTM, too.

Sat, Nov 18, 12:29 PM
rengolin added a comment to D40011: [TableGen] AsmMatcher: Fix bug with reported diagnostic for operand..

@olista01 is working in this area...

Sat, Nov 18, 12:27 PM
rengolin added a comment to D38676: [LV] Model masking in VPlan, introducing VPInstructions.

I finished my review, and apart from my two final comments, everything looks fine.

Sat, Nov 18, 12:20 PM

Thu, Nov 16

rengolin added a comment to D40137: [ARM] 't' asm constraint should accept i32.

Is the T constraint only used for movs?

Thu, Nov 16, 12:47 PM
rengolin added a reviewer for D35635: [ARM] Optimize {s,u}{add,sub}.with.overflow: peter.smith.
Thu, Nov 16, 12:45 PM

Wed, Nov 15

rengolin added inline comments to D38676: [LV] Model masking in VPlan, introducing VPInstructions.
Wed, Nov 15, 1:01 PM

Tue, Nov 14

rengolin accepted D39894: [AArch64][SVE] Asm: Report SVE parsing diagnostics only once.

Right, ok, you'll reuse the parser with a different kind later, that makes sense and the code is cleaner this way.

Tue, Nov 14, 6:45 PM

Nov 13 2017

rengolin added a comment to D39894: [AArch64][SVE] Asm: Report SVE parsing diagnostics only once.

tryParseSVERegister() will also handle Predicates when we extend matchRegisterNameAlias with SVE predicates in a later patch, similar to how we did it for data vectors and so there is no need for having the 'DataVector' part in the name.

Nov 13 2017, 12:34 PM

Nov 10 2017

rengolin added a comment to D39712: [ARM] Add an alias for psr and psr_nzcvq.

I'v looked everywhere and "psr" is not even pre-UAL. Please, fix the user code instead.

Nov 10 2017, 11:08 PM
rengolin accepted D36104: [AArch64] Coalesce Copy Zero during instruction selection.

Piggyback on @gberry's review, I can't see anything wrong with it, LGTM. Thanks!

Nov 10 2017, 10:33 PM
rengolin added a comment to D39894: [AArch64][SVE] Asm: Report SVE parsing diagnostics only once.

Is the VEDataVectorRegister` to SVERegister change meaningful?

Nov 10 2017, 10:27 PM
rengolin added a comment to D38378: [ARM] Optimize {s,u}{add,sub}.with.overflow..

The change itself looks good, I only have one question to clarify a part of the code, but otherwise seems fine.

Nov 10 2017, 10:25 PM
rengolin added a comment to D38175: [ARM] Make sure assembler rejects PC as an operand for VMOV.F16.

Just a nit, why did you call it rGPR2? If this is a noPC/noSP class, than a longer but more meaningful name like rGPRnopcsp would be preferable.

Nov 10 2017, 10:09 PM
rengolin requested changes to D39712: [ARM] Add an alias for psr and psr_nzcvq.

For reference, we tend not to add undocumented stuff. We only add GNU extensions when it's simple and necessary. We specifically don't add things that are implemented in GCC source code or test-cases, as that brings us bug-for-bug emulation, which we purposefully won't do.

Nov 10 2017, 9:34 PM
rengolin accepted D39068: [LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe.

LGTM, thanks!

Nov 10 2017, 9:25 PM
rengolin added a reviewer for D39599: [ARM] Fix incorrect conversion of a tail call to an ordinary call: peter.smith.
Nov 10 2017, 6:10 AM
rengolin accepted D39792: [AArch64][SVE] Asm: More concise test format.

One of the negative tests actually exposed a bug where the same diagnostic is produced several times (it does this because it is unmatched for each register operand class, rather than bail out with a parse failure when z31.x is given). I have a fix for that, but will address it in a separate patch so not to complicate this patch.

Nov 10 2017, 3:16 AM

Nov 8 2017

rengolin added a comment to D39792: [AArch64][SVE] Asm: More concise test format.

Much better indeed! I agree with Florian on the additional "bad arguments" tests. Thanks!

Nov 8 2017, 4:42 AM
rengolin accepted D39700: [Builtins] Do not use tailcall for Thumb1.

With respect to this review specifically, I think that the change should make compiler-rt no worse and for its likely use case of v6-m, there aren't any non-core registers for the C language functions to corrupt. With respect to the larger problem of many of the other __aeabi_ functions calling C functions I think we should address separately to this Review.

Nov 8 2017, 3:44 AM

Nov 7 2017

rengolin added a comment to D39700: [Builtins] Do not use tailcall for Thumb1.

This commit in musl has details on why calling memset, etc. doesn't work and violates the ABI:

https://git.musl-libc.org/cgit/musl/commit?id=e6def544358afd5648a428d2e02c147a1f901048

Nov 7 2017, 3:20 PM
rengolin added a comment to D39700: [Builtins] Do not use tailcall for Thumb1.

Looking further at this, it looks like your current implementations of the __aeabi_* functions are simply wrong - they clobber registers that the ABI does not allow them to clobber.

Nov 7 2017, 3:17 PM
rengolin updated subscribers of D39700: [Builtins] Do not use tailcall for Thumb1.

Given that compiler-rt will need to support more than one C-library (Musl, libc, newlib etc.) it probably has to assume that none of the __aeabi_ specialisations of memcpy, memset etc. are implemented by the C-library.

Nov 7 2017, 3:15 PM
rengolin added reviewers for D39346: [LV] [ScalarEvolution] Fix PR34965 - Cache pointer stride information before LV code gen: gilr, Ayal, hsaito.

I implemented a new fix where the return value of Legal->isConsecutivePtr() is collected just before the original loop is modified. This approach seems simpler and less invasive than the one described in my previous comment.

Nov 7 2017, 5:44 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

CTMark benchmarks (Baseline corresponds to non-patched version)


CFLAGS:

-target armv7-unknown-linux-gnueabihf  -O3 -mcpu=cortex-a15 -fomit-frame-pointer
Nov 7 2017, 2:41 AM

Nov 6 2017

rengolin accepted D39091: [AArch64][SVE] Asm: Add support for (ADD|SUB)_ZZZ.

Hi Sander,

Nov 6 2017, 9:32 AM
rengolin accepted D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC).
Nov 6 2017, 9:27 AM
rengolin accepted D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support.

LGTM, without the extra whitespace change. Thanks!

Nov 6 2017, 9:26 AM
rengolin added a comment to D39068: [LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe.

Excellent, ok, this patch is looking good to me now. How does this fit into the whole story?

Nov 6 2017, 9:19 AM

Nov 3 2017

rengolin added a comment to D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC).

This looks good to me, thanks!

Nov 3 2017, 8:42 AM
rengolin added a comment to D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support.

Two nits, and I'm happy. :)

Nov 3 2017, 8:42 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

Actually it had but not many.
There are some other regressions/improvements. I have not listed them because the hottest code is not changed.

Nov 3 2017, 8:26 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

The LNT server does not provide compile time reports. I am not sure whether they are not displayed or no compile time data available.
Also compilation happens on a target. This is not cross-compilation. I don't know how compile time is stable on Juno boards.

Nov 3 2017, 7:34 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

I've got first results of benchmark runs: the LNT test suite + a private benchmark 01. I used the latest patch.
The configuration is a Juno board Cortex-A57/A53, v8-a, AArch32, Thumb2.
Options: -O3 -mcpu=cortex-a57 -mthumb -fomit-frame-pointer
The runs passed without errors.

Nov 3 2017, 5:18 AM

Nov 2 2017

rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

I've shown benchmarks on few Android phones, if you remember.

Nov 2 2017, 12:05 PM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

I believe algorithm complexity is O(N), where N is a number of nodes in Selection DAG (it's about finding a minimum value in a list of values). You're suggesting O(N*logN), why?

Nov 2 2017, 11:28 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

The only user of Addr operand (VST1_UPD) is Store(Value, Addr). What should I do next?

Nov 2 2017, 10:30 AM
rengolin added a comment to D39558: [TargetParser][AArch64] Reorder enum to preserve 5.0.0 libLLVM ABI..

Why did they change in the first place?

The reordering was arbitrary, but I didn't realize the implications at the time. :/

Nov 2 2017, 9:51 AM
rengolin accepted D39558: [TargetParser][AArch64] Reorder enum to preserve 5.0.0 libLLVM ABI..

Ha, I see. That was your commit trying to reorder things. :) LGTM.

Nov 2 2017, 9:08 AM
rengolin added a comment to D39558: [TargetParser][AArch64] Reorder enum to preserve 5.0.0 libLLVM ABI..

Why did they change in the first place?

Nov 2 2017, 8:58 AM
rengolin added a reviewer for D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions: eli.friedman.
Nov 2 2017, 8:47 AM
rengolin added a reviewer for D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions: majnemer.

After looking at your PDF, it sounds as though you don't need the while loop at all.

Nov 2 2017, 8:46 AM
rengolin added inline comments to D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support.
Nov 2 2017, 7:01 AM
rengolin added inline comments to D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support.
Nov 2 2017, 3:23 AM

Nov 1 2017

rengolin added a comment to D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC).

Small nit, but overall, looks good to me.

Nov 1 2017, 11:43 AM
rengolin added a comment to D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support.

Apart from the one comment about identifying the register as SVE specific, the patch looks good.

Nov 1 2017, 11:24 AM

Oct 31 2017

rengolin added inline comments to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.
Oct 31 2017, 12:28 PM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

Oct 31 2017, 9:05 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

@javed.absar @rengolin
I've created a PowerPoint file describing algorithm. Hope this helps.

Oct 31 2017, 9:04 AM
rengolin accepted D39090: [AArch64][SVE] Asm: Set SVE as unsupported feature for existing scheduler models.
Oct 31 2017, 7:16 AM
rengolin accepted D39087: [AArch64][SVE] Asm: Extend EnforceVectorSubVectorTypeIs to distinguish Scalable Vectors.

This one is trivial, I agree.

Oct 31 2017, 7:16 AM
rengolin added a comment to D39090: [AArch64][SVE] Asm: Set SVE as unsupported feature for existing scheduler models.

Yes, this change makes sense to me.

Oct 31 2017, 6:48 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

Samsung Galaxy Nexus (TI OMAP 4460 Dual-core Cortex-A9 1,2 GHz):
Huawei Honor 8 (Cortex A72, 2.3GHz):

Oct 31 2017, 5:50 AM
rengolin committed rL316991: Merge r311574: ARM: explicitly specify the 8-byte alignment.
Merge r311574: ARM: explicitly specify the 8-byte alignment
Oct 31 2017, 5:22 AM
rengolin added inline comments to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.
Oct 31 2017, 5:13 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

I'll do AArch32/AArch64 benchmarks runs on our Juno boards. It would be worth to run CTMark to check compilation time.

Oct 31 2017, 5:09 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.

With patch:

real	1m40.086s

W/o patch:

real	1m40.619s
Oct 31 2017, 2:00 AM

Oct 30 2017

rengolin added inline comments to D39068: [LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe.
Oct 30 2017, 11:09 AM
rengolin added a comment to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.


Sharing my matrix multiplication example, if anyone's interested.

Oct 30 2017, 10:58 AM
rengolin added inline comments to D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions.
Oct 30 2017, 7:26 AM
rengolin added reviewers for D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions: rovka, evandro, mcrosier, kristof.beyls.

So, this is an interesting change, on the TODO list for a while, but the code and the tests are changing too much in areas where not clearly related.

Oct 30 2017, 6:46 AM

Oct 28 2017

rengolin added a dependent revision for D38676: [LV] Model masking in VPlan, introducing VPInstructions: D39068: [LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe.
Oct 28 2017, 10:37 AM
rengolin added a dependency for D39068: [LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe: D38676: [LV] Model masking in VPlan, introducing VPInstructions.
Oct 28 2017, 10:37 AM
rengolin added a comment to D39068: [LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe.

The patch is much easier to review now, mostly code movement. Thanks!

Oct 28 2017, 9:06 AM

Oct 26 2017

rengolin committed rL316664: Merging r316657: fixing libunwind tests.
Merging r316657: fixing libunwind tests
Oct 26 2017, 6:53 AM

Oct 25 2017

rengolin accepted D37484: [libunwind] Always use unwind tables in tests.

Hi Peter,

Oct 25 2017, 11:50 AM
rengolin added inline comments to D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support.
Oct 25 2017, 7:55 AM
rengolin added inline comments to D39091: [AArch64][SVE] Asm: Add support for (ADD|SUB)_ZZZ.
Oct 25 2017, 6:54 AM
rengolin accepted D38588: Clear LastMappingSymbols and LastEMS(Info) when resetting the ARM(AArch64)ELFStreamer.

Sorry, fell out of my radar.

Oct 25 2017, 6:36 AM

Oct 24 2017

rengolin added reviewers for D39091: [AArch64][SVE] Asm: Add support for (ADD|SUB)_ZZZ: kristof.beyls, mcrosier, evandro.

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

Oct 24 2017, 10:31 AM
rengolin added reviewers for D39090: [AArch64][SVE] Asm: Set SVE as unsupported feature for existing scheduler models: kristof.beyls, mcrosier, evandro.

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

Oct 24 2017, 10:30 AM
rengolin added reviewers for D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support: kristof.beyls, mcrosier, evandro.

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

Oct 24 2017, 10:30 AM
rengolin added reviewers for D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC): kristof.beyls, mcrosier, evandro.

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

Oct 24 2017, 10:30 AM
rengolin added reviewers for D39087: [AArch64][SVE] Asm: Extend EnforceVectorSubVectorTypeIs to distinguish Scalable Vectors: kristof.beyls, mcrosier, evandro.

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

Oct 24 2017, 10:29 AM
rengolin added a comment to D39090: [AArch64][SVE] Asm: Set SVE as unsupported feature for existing scheduler models.

Simple and straightforward. Looks good, but will approve once all patches are good.

Oct 24 2017, 10:27 AM
rengolin added a comment to D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support.

Overall, this looks pretty straightforward to me.

Oct 24 2017, 10:25 AM
rengolin added inline comments to D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC).
Oct 24 2017, 10:13 AM
rengolin added inline comments to D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC).
Oct 24 2017, 10:11 AM
rengolin accepted D39237: [ARM] Error for invalid shift in memory operand.

LGTM. Thanks!

Oct 24 2017, 6:35 AM
rengolin accepted D39238: [ARM] Tighten up CHECK lines in a test.

LGTM. Thanks!

Oct 24 2017, 6:35 AM
rengolin added a reviewer for D39196: [ARM] Remove pre-UAL FLDM/FSTM aliases: joerg.

Adding @joerg, who sometimes deals in old Entish ARM dialects.

Oct 24 2017, 2:59 AM
rengolin accepted D39194: [ARM] Include operand class name in dev diags.

LGTM. Thanks!

Oct 24 2017, 2:20 AM

Oct 23 2017

rengolin added a comment to D39194: [ARM] Include operand class name in dev diags.

Right, looking back, this is the only other use of DevDiags. At all.

Oct 23 2017, 4:00 PM
rengolin added a comment to D39194: [ARM] Include operand class name in dev diags.

Normal users will only ever see the "invalid operand for instruction" part of the diagnostic, regardess of whether they have assertions enabled or not. The extra info is only enabled when the -arm-arm-parser-dev-diags option is used (the "if (DevDiags)" check above), which isn't even exposed as a clang option (you'd need to pass it through with -mllvm).

Oct 23 2017, 3:07 PM
rengolin requested changes to D39192: [ARM] processInstruction must return true if it makes a change.

It can go in as part of a patch set and it will still avoid the failures.

Oct 23 2017, 3:03 PM
rengolin added a comment to D39194: [ARM] Include operand class name in dev diags.

Why do you think this should be kept downstream? It's a debug option which I've found useful while adding diagnostics for assembly options, none of which I plan on keeping downstream.

Oct 23 2017, 2:49 PM
rengolin added a comment to D39192: [ARM] processInstruction must return true if it makes a change.

Right, it would be nice to have that as a precursor to the patch set that you're working on, so that you can claim that this is necessary (and show proof with following patches), instead of having it loose like that.

Oct 23 2017, 2:44 PM
rengolin accepted D39190: [ARM] tSETEND needs IsThumb.

It's _not_ OK to select the Thumb version of this instruction in ARM mode, and that's what this patch is preventing.

Oct 23 2017, 2:43 PM