- User Since
- Jul 18 2017, 1:54 PM (95 w, 6 d)
Wed, May 1
LGTM, thanks Sanjay
Tue, Apr 30
Apr 18 2019
Feb 6 2019
Feb 5 2019
Migrated flag copy utility function to MachineInstr as a static function.
Feb 4 2019
Feb 1 2019
Jan 28 2019
Dec 18 2018
Dec 17 2018
Updated for posterity with the post review changes...
Dec 16 2018
As requested, add one more component for copy ir flags in to this review
Dec 13 2018
Oct 30 2018
Sep 19 2018
The fixed test for WebAssembly is in rL342598.
Ok, looking into it now.
Sep 13 2018
Changed copyIRFlags as suggested...
Sep 12 2018
Sep 11 2018
Collapsed for single version...
Hal/Sanjay could one of you have a look at this code, it would be great to wrap it up...
Sep 10 2018
All the nsw/nuw context is set above SDAG currently in code that is common to both GlobalIsel and SDAG, meaning adding them to MI should be analogous to SDAG as a pass through and for detection of already derived context for actions and side effects. Currently SDAG does have some DAG combining opts that utilize these flags. MI should too for GlobalIsel to map existing optimizations and functionality, else GlobalIsel will be handicapped. Having said that, if we do due diligence to map SDAG context in GlobalIsel, then there should be no new poison state, only that which currently exists. We should also keep the door open for new SDAG optimizations under nsw/nuw to map into that space to provide parity of support.
Sep 8 2018
The flags were requested for support in GlobalIsel, where there is a plan to make use of them once they are available. The path through SelectionDAG could also make use of the flags.
Sep 6 2018
With the appropriate test file...
I have a test case which will show shortly...
Sep 4 2018
I added one more non fmf instruction and there is room to add others if needed.
Thanks, that is a good alternative. I am testing it internally to see I get similar coverage. More later...
Aug 31 2018
In the newly created test case above, as checked in for test/CodeGen/X86/intersect-fma-fmf.ll, we get precisely this scenario. From a debug session breakpointed in llvm::SelectionDAGBuilder::visit at the line containing Node->intersectFlagsWith(IncomingFlags);
The Instruction in question is: %tmp11 = extractelement <4 x float> %tmp10, i32 1 as seen by the debugger, the Node we are associating it to is: t7: f32 = fadd nnan ninf nsz arcp contract afn reassoc t6, t5.
The extractelement is an Instruction that never carries flags and qualifies under
Another data point, this change caused no drift in the lit tests, meaning none of our FMF opts managed in the lit tests are modified by the behavior.
Correctly stated, but out of context. Some clarity first. The kinds of instructions in our case are neither strict nor fast, they never carry flags. Neither scalarization nor partial vectorization should cause FMF obliteration. Perhaps we need additional mechanisms?
Aug 30 2018
With the test file sync'd
I have a smaller test case now, its baseline analog should show up in the tree in a bit...
without the change:
I will have both with and without assembler example shortly, added here. As for the test case, it is pretty close to minimal with context needed for association, but I will see if I can shrink it a bit after the assembler is up.
Copying all available flags as false should not change the initial state of the object after its creation. It only does so because we are using AnyDefined as a sticky bit for assignment of any flag regardless of value. My notion is that any true value indicates that at least a value is set to something other than a default value (false). The test conditions occurs in partial vectorization and in our case, after scalarization on gpus where interior math operations occur. It happens frequently in our cases. The problem is when an instruction is associated to a Node that does have flags and where the Instruction it is associated to does not. This only ever occurs where the association is disjunct, meaning the Node and the Instruction originated differently. Generally its the other way around, where Node is empty and we are inheriting flags from an Instruction as the Instruction originated the Node in question. All other operations of intersect are done from Node to Node, ergo this change does not modify that behavior, that only occurs in this context.
Aug 22 2018
The attached test is a x86 approximation of internal tests, altered for platform, which show this problem which occurs frequently on many FMF optimization contexts.
Aug 20 2018
Actually the other test in rL339938 shows the positive side, where both binops are fast to show the intersection in /test/Transforms/LoopVectorize/reduction.ll
The select_meta test has a directly related case in it that maps to the bug test case in https://bugs.llvm.org/show_bug.cgi?id=38641
Aug 16 2018
Aug 9 2018
Aug 8 2018
With the flags added...
At this point there isn't any code either way that checks for fneg wrt to peering through or mining flags...
Its either that or peer through them like fp_round and fp_extend, which seems more natural since these usually camp over arithmetics.
With changes to the new fold tests you added and the code mod.
I see what you meant, removing isNegatibleForFree/GetNegatedExpression in favor of fneg when that is what we doing. I have the rest of the change ready now. Should be up in a bit...
Even with all the above changes, the opportunity persists. We need to move:
Aug 7 2018
Looked it over in detail, the transformations all track.
LGTM, up to you if you want second set of eyes.
Updated and sync'd with fsub tests from r339197.
With the non const version for both tests and some cleanup.
Nope, we can do either, I will upload the non constant version. Also I will split fp_fold.ll to use just unsafe for the initial check as NFC before this change.
I like these test versions a little better, they show an opportunity yet to be had for the IR optimizer.
Aug 2 2018
Aug 1 2018
Jun 18 2018
Updated for rL334984
This change does not modify testing...
D48289 is now required to be checked in before this change.
Jun 15 2018
Updated comment for reassociation case
This review is now covered in other checkins.